Bug 161708 – Make HasOwnProperty faster

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

Speedometer spend around 2-2.5% of its time in HasOwnProperty. Lets try to make this faster to speed up speedometer.

[Saam Barati]

Created attachment 288996 [details]
WIP

Looks to be a 1-2% speedometer progression.

[Saam Barati]

Created attachment 289050 [details]
WIP

works with symbols now too. I need to implement 32-bit now.

[Build Bot]

Comment on attachment 289050 [details]
WIP

Attachment 289050 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2086449

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289052 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289050 [details]
WIP

Attachment 289050 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2086453

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289053 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289050 [details]
WIP

Attachment 289050 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2086454

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289054 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289050 [details]
WIP

Attachment 289050 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2087121

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289056 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Saam Barati]

Created attachment 289196 [details]
patch

[WebKit Commit Bot]

Attachment 289196 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:77:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:104:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 289196 [details]
patch

Attachment 289196 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2098907

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289197 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289196 [details]
patch

Attachment 289196 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2098911

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289198 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289196 [details]
patch

Attachment 289196 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2098910

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289199 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289196 [details]
patch

Attachment 289196 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2098916

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289200 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Saam Barati]

Created attachment 289203 [details]
patch

Oops I'm crashing when the VM is deallocated. I need to override operator delete on HasOwnPropertyCache to call fastFree.

[WebKit Commit Bot]

Attachment 289203 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:77:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:104:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 289204 [details]
patch

fix style of `if`

[WebKit Commit Bot]

Attachment 289204 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:103:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 289204 [details]
patch

Attachment 289204 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2101127

New failing tests:
fast/dom/Window/forbid-showModalDialog.html

[Build Bot]

Created attachment 289205 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289204 [details]
patch

Attachment 289204 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2101133

New failing tests:
fast/dom/Window/forbid-showModalDialog.html

[Build Bot]

Created attachment 289206 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289204 [details]
patch

Attachment 289204 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2101132

New failing tests:
fast/dom/Window/forbid-showModalDialog.html

[Build Bot]

Created attachment 289207 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289204 [details]
patch

Attachment 289204 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2101137

New failing tests:
fast/dom/Window/forbid-showModalDialog.html

[Build Bot]

Created attachment 289208 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Saam Barati]

Looks like there is some crazy stuff going on with showModalDialogue in JSDOMWindow. Let me see what's happening there.

[Saam Barati]

I think this patch is still in good shape for review. It's failing just one showModalDialogue test, and I think the bug might actually be in showModalDialogue populating a PropertySlot that claims its cacheable.

[Saam Barati]

(In reply to comment #34)
> I think this patch is still in good shape for review. It's failing just one
> showModalDialogue test, and I think the bug might actually be in
> showModalDialogue populating a PropertySlot that claims its cacheable.

I'm still trying to figure out exactly what to do about this. I'll either work around it in my patch somehow, or I'll make a change to how JSDOMWindow handle's the showModalDialogue property. I suspect it'll be the latter.

[Yusuke Suzuki]

Comment on attachment 289204 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289204&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2275
> +#else // CPU(slot) || (CPU(ARM) && CPU(ARM_HARDFP)) || CPU(ARM64) || CPU(MIPS) || CPU(SH4)

Seems incorrect.

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:34
> +    static const uint32_t size = 8 * 1024;

Is it necessary to allocate this large size cache?

[Saam Barati]

Comment on attachment 289204 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289204&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2275
>> +#else // CPU(slot) || (CPU(ARM) && CPU(ARM_HARDFP)) || CPU(ARM64) || CPU(MIPS) || CPU(SH4)
> 
> Seems incorrect.

Oops. I'll revert

>> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:34
>> +    static const uint32_t size = 8 * 1024;
> 
> Is it necessary to allocate this large size cache?

Let me try to mess around with the size again, I only varied the size in the beginning of me writing this patch

[Geoffrey Garen]

Comment on attachment 289204 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289204&action=review

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:69
> +    ALWAYS_INLINE Optional<bool> get(Structure* structure, PropertyName propName)

Is the C++ implementation a win, or should we only have a JIT implementation?

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:88
> +        if (!slot.isCacheable() && !slot.isUnset())
> +            return;

Why the && here?

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:118
> +ALWAYS_INLINE HasOwnPropertyCache* VM::ensureHasOwnPropertyCache()
> +{
> +    if (UNLIKELY(!m_hasOwnPropertyCache))
> +        m_hasOwnPropertyCache = std::unique_ptr<HasOwnPropertyCache>(HasOwnPropertyCache::create());
> +    return m_hasOwnPropertyCache.get();
> +}

I'm kind of contradicting myself here, but I wonder if the cache should be per-structure instead of global.

Reasons to prefer:

(1) No need to check structure equality on lookup -- faster;

(2) Simpler lifetime rules (similar to property name array cache);

(3) Reduction in collisions -- higher hit rate.

Reasons not to prefer:

(1) Uses more memory if you hasOwnProperty on lots of objects;

[Saam Barati]

Comment on attachment 289204 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289204&action=review

>> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:69
>> +    ALWAYS_INLINE Optional<bool> get(Structure* structure, PropertyName propName)
> 
> Is the C++ implementation a win, or should we only have a JIT implementation?

Not sure. I remember not getting a win in speedometer, but I'll see if it helps for micro benchmarks. I'll also run without JIT implementation for speedometer and measure time spent using the sampling profiler, to see if it sees less time spent in HasOwnProperty.

>> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:88
>> +            return;
> 
> Why the && here?

unset slots always return false from isCacheable. So we're basically asking if it's set and uncacheable.
If it's unset, we have other guards to see if we can cache it below, using `structure->propertyAccessesAreCacheableForAbsence()`.
It might be easier to read the if as:
if (!(slot.isCacheable() || slot.isUnset())) ...

>> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:118
>> +}
> 
> I'm kind of contradicting myself here, but I wonder if the cache should be per-structure instead of global.
> 
> Reasons to prefer:
> 
> (1) No need to check structure equality on lookup -- faster;
> 
> (2) Simpler lifetime rules (similar to property name array cache);
> 
> (3) Reduction in collisions -- higher hit rate.
> 
> Reasons not to prefer:
> 
> (1) Uses more memory if you hasOwnProperty on lots of objects;

Geoff and I discussed offline another possible use of the cache: It may help property access in other scenarios of we have the cache be used per structure
and we can optimize the memory overhead of Structure's property table by making it an array of sorts and putting the cache in front of it.
If we consider a world where we don't think of the cache being used to optimize other types of property access, and only consider it only
in a world of HasOwnProperty, I think the global cache is strictly faster (at least for the benchmark I'm tuning for in Speedometer, and probably
other real use cases of the HasOwnProperty where it will be unlikely to encounter very many structures in hot invocations of HasOwnProperty).

1) I don't think this will actually be faster since speedometer has about a 92-94% hit rate in the cache. The reason being, we'd need to do three extra
dependent loads and two extra branches: first load the structure from the object, and then to load and check if it has rare data, then to load the cache from the rare data
and check if it is non-null. With a global cache, the machine code we emit just materializes the global cache pointer.

2) Yeah, the rules are probably a bit simpler. They're definitely much cleaner if tied to a Structure's lifetime. However, the lifetime rules now
aren't very complicated either. They're not super elegant, since we just clear the cache on every GC, but I don't think they're too difficult to understand.

3) This will definitely be true, especially for programs that encounter very many structures. I think for programs that don't encounter many structures, it's probably
a toss up. I also think it's likely that most programs won't see very many structure's in a hot hasOwnProperty.

Not to prefer:
1) I think this could go either way actually. If a program encounters very few structures in hasOwnProperty, we will probably save memory. If it encounters many 
Structures, we'll definitely use more memory. However, we'll also get better performance in the case where we encounter very many Structures.

[Saam Barati]

Created attachment 289265 [details]
patch

I think this fixes layout test failures.

[WebKit Commit Bot]

Attachment 289265 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:107:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 289272 [details]
patch

Reduce the cache size to 2*1024. I don't see a reduction in hit rate on Speedometer with this, so it's better to use less memory if possible.

[WebKit Commit Bot]

Attachment 289272 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:107:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 289272 [details]
patch

r=me

[Saam Barati]

Created attachment 289293 [details]
patch for landing

[WebKit Commit Bot]

Attachment 289293 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:107:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/206136

[Csaba Osztrogonác]

(In reply to comment #47)
> landed in:
> https://trac.webkit.org/changeset/206136

There are many failiures on 32 bit bots after this change.
Please see build.webkit.org for details.

[Saam Barati]

(In reply to comment #48)
> (In reply to comment #47)
> > landed in:
> > https://trac.webkit.org/changeset/206136
> 
> There are many failiures on 32 bit bots after this change.
> Please see build.webkit.org for details.

Looking now. Thanks

[Saam Barati]

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #47)
> > > landed in:
> > > https://trac.webkit.org/changeset/206136
> > 
> > There are many failiures on 32 bit bots after this change.
> > Please see build.webkit.org for details.
> 
> Looking now. Thanks

I know what the fix is, just building a testing locally before committing.

[Saam Barati]

Landed x86 32 bit fix in:
https://trac.webkit.org/changeset/206149