Bug 164357 – EvalCodeCache should not give up in strict mode and other cases

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

EvalCodeCache should not give up in strict mode and other cases

[Geoffrey Garen]

Created attachment 293745 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

Is that failure real? The patch makes sense to me.

[Saam Barati]

Can you add some microbenchmarks that shows a speed up with block scoping? Maybe also some correctness tests for block scoping/with scoping

[Geoffrey Garen]

Created attachment 293827 [details]
Patch

[Geoffrey Garen]

New patch includes a micro benchmark and correctness test.

[Saam Barati]

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?

[Geoffrey Garen]

> 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.

[Saam Barati]

(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;

[Build Bot]

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

[Build Bot]

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

[Geoffrey Garen]

Looks like some of the original EWS failures were fake, but the hang in js/dom/domjit-accessor-polymorphic.html is real.

[Geoffrey Garen]

Created attachment 293934 [details]
Patch

[Michael Saboff]

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.

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

Committed r208404: <http://trac.webkit.org/changeset/208404>

[Chris Dumez]

There was a 10% Sunspider regression in the range r208404-r208416, could it be caused by this patch?

[Chris Dumez]

(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 :)

[Chris Dumez]

(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

[Chris Dumez]

(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.

[Geoffrey Garen]

Yes, this patch caused a performance regression. Tracked here: https://bugs.webkit.org/show_bug.cgi?id=164499.