EvalCodeCache should not give up in strict mode and other cases
Created attachment 293745 [details] Patch
Comment on attachment 293745 [details] Patch Attachment 293745 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2454760 New failing tests: js/dom/domjit-accessor-polymorphic.html
Created attachment 293759 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Is that failure real? The patch makes sense to me.
Can you add some microbenchmarks that shows a speed up with block scoping? Maybe also some correctness tests for block scoping/with scoping
Created attachment 293827 [details] Patch
New patch includes a micro benchmark and correctness test.
Comment on attachment 293827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293827&action=review r=me > JSTests/ChangeLog:8 > + * microbenchmarks/eval-cached.js: Added. 45x faster now. lol, nice > Source/JavaScriptCore/bytecode/EvalCodeCache.h:90 > + uint32_t m_callSiteIndex; Why not use an actual CallSiteIndex instead of a uint32_t?
> Why not use an actual CallSiteIndex instead of a uint32_t? I'm a CallSiteIndex noob. It looked to me like CallSiteIndex is a way to represent "CallSiteIndex or NULL". Is that right? I don't every want the NULL variant, so I thought uint32_t was the more specific type.
(In reply to comment #9) > > Why not use an actual CallSiteIndex instead of a uint32_t? > > I'm a CallSiteIndex noob. It looked to me like CallSiteIndex is a way to > represent "CallSiteIndex or NULL". Is that right? I don't every want the > NULL variant, so I thought uint32_t was the more specific type. It's true that some CallSiteIndex can be empty, however, just because that's allowed, I don't think making the field a CallSiteIndex makes it a less specific type. I actually think it's still a more specific type because not all uint32_t are a CallSiteIndex, but all CallSiteIndex are backed by a single uint32_t. I think it's OK to just mandate that no empty CallSiteIndex is passed to the cache. To me as a reader of the code, it's easier to understand a field shaped like: CallSiteIndex m_callSiteIndex; instead of uint32_t m_callSiteIndex;
Comment on attachment 293827 [details] Patch Attachment 293827 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2457558 New failing tests: js/dom/domjit-accessor-polymorphic.html
Created attachment 293845 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Looks like some of the original EWS failures were fake, but the hang in js/dom/domjit-accessor-polymorphic.html is real.
Created attachment 293934 [details] Patch
Comment on attachment 293934 [details] Patch r=me NIT - It looks like some functions have the evalSource as the first argument and the callSiteIndex as the second argument, while others have the order flipped. I think it would be cleaner if all functions used the same order.
> NIT - It looks like some functions have the evalSource as the first argument > and the callSiteIndex as the second argument, while others have the order > flipped. I think it would be cleaner if all functions used the same order. Fixed.
Committed r208404: <http://trac.webkit.org/changeset/208404>
There was a 10% Sunspider regression in the range r208404-r208416, could it be caused by this patch?
(In reply to comment #18) > There was a 10% Sunspider regression in the range r208404-r208416, could it > be caused by this patch? This really seems like the most likely candidate in the range. Also note that the most regressed subtest is date-format-tofte which is 160% regressed :)
(In reply to comment #19) > (In reply to comment #18) > > There was a 10% Sunspider regression in the range r208404-r208416, could it > > be caused by this patch? > > This really seems like the most likely candidate in the range. Also note > that the most regressed subtest is date-format-tofte which is 160% regressed > :) rdar://problem/29164133
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > There was a 10% Sunspider regression in the range r208404-r208416, could it > > > be caused by this patch? > > > > This really seems like the most likely candidate in the range. Also note > > that the most regressed subtest is date-format-tofte which is 160% regressed > > :) > > rdar://problem/29164133 Likely a 5% JetStream regression as well.
Yes, this patch caused a performance regression. Tracked here: https://bugs.webkit.org/show_bug.cgi?id=164499.