Speedometer spend around 2-2.5% of its time in HasOwnProperty. Lets try to make this faster to speed up speedometer.
Created attachment 288996 [details] WIP Looks to be a 1-2% speedometer progression.
Created attachment 289050 [details] WIP works with symbols now too. I need to implement 32-bit now.
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.
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
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.
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
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.
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
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.
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
Created attachment 289196 [details] patch
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.
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.
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
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.
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
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.
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
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.
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
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.
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.
Created attachment 289204 [details] patch fix style of `if`
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.
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
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
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
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
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
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
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
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
Looks like there is some crazy stuff going on with showModalDialogue in JSDOMWindow. Let me see what's happening there.
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.
(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.
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?
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
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;
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.
Created attachment 289265 [details] patch I think this fixes layout test failures.
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.
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.
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.
Comment on attachment 289272 [details] patch r=me
Created attachment 289293 [details] patch for landing
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.
landed in: https://trac.webkit.org/changeset/206136
(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.
(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
(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.
Landed x86 32 bit fix in: https://trac.webkit.org/changeset/206149