Issue 11848043: Test (without repair) for V8 increment operator bug. (Closed)
|
Created:
12 years, 7 months ago by kpreid2 Modified:
12 years, 7 months ago CC:
caja-discuss-undisclosed_googlegroups.com, MarkM, felix8a, ihab.awad, Jasvir, metaweta, MikeSamuel Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionDetects https://code.google.com/p/v8/issues/detail?id=2779
[Incorporated into r5514.]
Patch Set 1 #
Total comments: 5
MessagesTotal messages: 9
I'm putting this out to get it reviewed, but I hope it will be combined with Ihab's rewriter-based repair.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:2015: for (var j = 0; j < 100000; j++) {} How much does this slow down overall SES startup? Can we guard this with an earlier cheaper test so that platforms not in danger of this bug can avoid the overhead of this test?
Sign in to reply to this message.
https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:2015: for (var j = 0; j < 100000; j++) {} On 2013/07/25 19:04:50, MarkM wrote: > How much does this slow down overall SES startup? 0.71 ms, according to the Chrome profiler. It is in third place compared to the most expensive tests, test_FREEZING_BREAKS_PROTOTYPES and test_FREEZE_IS_FRAME_DEPENDENT, which together take 14 ms. repairES5.js takes a total of 78 ms. (Individual test timings are for the pre-repair testing; run in a loop with reported times adjusted.) > Can we guard this with an earlier cheaper test so that platforms not in danger > of this bug can avoid the overhead of this test? As far as I know, this is the only way to provoke the bug. Do you have any ideas for an early test? I think it would have to be essentially "detect V8".
Sign in to reply to this message.
https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:2015: for (var j = 0; j < 100000; j++) {} One way might be to add a conditional inside the loop and break out of it early if a change is detected. From the V8 bug thread, iirc, it only took tens of iterations to find it. Maybe compare the loop with the conditional and the loop without? Also maybe 100000 is overkill? ;)
Sign in to reply to this message.
https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:2015: for (var j = 0; j < 100000; j++) {} On 2013/07/25 22:32:40, ihab.awad wrote: > One way might be to add a conditional inside the loop and break out of it early > if a change is detected. This loop isn't making any change. It's just forcing V8 to think it's worth optimizing this function. > From the V8 bug thread, iirc, it only took tens of > iterations to find it. That was expressed as speculation, and insofar as I tried it and it worked, it worked only in non-strict mode (which is a different code path because then the increment throws in the early non-optimized runs). > Maybe compare the loop with the conditional and the loop without? > > Also maybe 100000 is overkill? ;) I tried smaller numbers (well, 10000) and it did not exhibit the bug. Look — I'm not a V8 guru. I've tried obvious simplifications and they don't work. If we want a better test, we should ask the Chrome folks.
Sign in to reply to this message.
https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/11848043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:2015: for (var j = 0; j < 100000; j++) {} On 2013/07/25 20:51:54, kpreid2 wrote: > On 2013/07/25 19:04:50, MarkM wrote: > > How much does this slow down overall SES startup? > > 0.71 ms, according to the Chrome profiler. It is in third place compared to the > most expensive tests, test_FREEZING_BREAKS_PROTOTYPES and > test_FREEZE_IS_FRAME_DEPENDENT, which together take 14 ms. repairES5.js takes a > total of 78 ms. > > (Individual test timings are for the pre-repair testing; run in a loop with > reported times adjusted.) > > > Can we guard this with an earlier cheaper test so that platforms not in danger > > of this bug can avoid the overhead of this test? > > As far as I know, this is the only way to provoke the bug. Do you have any ideas > for an early test? I think it would have to be essentially "detect V8". Given that this is so much less significant than test_FREEZING_BREAKS_PROTOTYPES and test_FREEZE_IS_FRAME_DEPENDENT, let's not worry about the time on this one until we've done something about the time on those. Well after this CL, we might want to add a property to each repair description stating which browsers that problem has ever been seen on, and consider running only those tests in production that are relevant to a given browser. In development, we should continually try all tests against all browsers, in order to update these candidate lists. Yes, this would be scary. But 78ms is much too slow.
Sign in to reply to this message.
sgtm!
Sign in to reply to this message.
Ok, so the following function under Chrome (Version 29.0.1547.22 beta) manifests
the bug:
var o = {x: 5};
function f(o) { o.x++; }
Object.freeze(o);
for (var i = 0; i < 10000; i++) { f(o); }
o.x;
The following does *not* manifest the bug:
(function() {
'use strict';
var o = {x: 5};
function f(o) { try { o.x++; } catch (e) {} }
Object.freeze(o);
for (var i = 0; i < 10000; i++) { f(o); }
return o.x;
})();
and neither does the following:
(function() {
'use strict';
var o = {x: 5};
function f(o) { o.x++; }
Object.freeze(o);
for (var i = 0; i < 10000; i++) { try { f(o); } catch (e) {} }
return o.x;
})();
So: do we really need to bother with this? I'll ask on the V8 bug thread.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||
