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
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
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
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
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.
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
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
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
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.
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.
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
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
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
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
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.
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.
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.
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.
(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.
2016-09-15 13:56 PDT, Saam Barati
2016-09-16 01:26 PDT, Saam Barati
2016-09-16 03:17 PDT, Build Bot
2016-09-16 03:21 PDT, Build Bot
2016-09-16 03:26 PDT, Build Bot
2016-09-16 05:34 PDT, Build Bot
2016-09-18 01:27 PDT, Saam Barati
2016-09-18 02:27 PDT, Build Bot
2016-09-18 02:32 PDT, Build Bot
2016-09-18 02:35 PDT, Build Bot
2016-09-18 02:41 PDT, Build Bot
2016-09-18 10:54 PDT, Saam Barati
2016-09-18 10:56 PDT, Saam Barati
2016-09-18 12:04 PDT, Build Bot
2016-09-18 12:08 PDT, Build Bot
2016-09-18 12:11 PDT, Build Bot
2016-09-18 12:17 PDT, Build Bot
2016-09-19 15:12 PDT, Saam Barati
2016-09-19 16:19 PDT, Saam Barati
2016-09-19 17:43 PDT, Saam Barati