JSGlobalObject's m_customGetterFunctionMap and m_customSetterFunctionMap should be sets, not maps, and should use both the identifier and function pointer as the key. This will allow WebCore to use the same custom getter and setter functions for multiple attributes, as is needed for CSSStyleDeclaration.
(In reply to Sam Weinig from comment #0) > JSGlobalObject's m_customGetterFunctionMap and m_customSetterFunctionMap > should be sets, not maps, and should use both the identifier and function > pointer as the key. Could please expand why they need to be sets, and if so, how JSFunctions would be stored? Agreed on the second part though.
(In reply to Alexey Shvayka from comment #1) > (In reply to Sam Weinig from comment #0) > > JSGlobalObject's m_customGetterFunctionMap and m_customSetterFunctionMap > > should be sets, not maps, and should use both the identifier and function > > pointer as the key. > > Could please expand why they need to be sets, and if so, how JSFunctions > would be stored? Agreed on the second part though. Right, I forgot to explain that part :). If we just change m_customGetterFunctionMap (and m_customSetterFunctionMap) from: WeakGCMap<GetValueFunc, JSCustomGetterFunction> to WeakGCMap<std::pair<PropertyName, GetValueFunc>, JSCustomGetterFunction> we would increase the size of the maps considerably. But, if we can use a set instead, we could take advantage of the fact that the JSCustomGetterFunction already stores the PropertyName and GetValueFunc. And thus, rather than increase size, we could actually make these a bit smaller, since the buckets would now just be Weak<JSCustomGetterFunction>. The problem is there isn't a WeakGCSet class yet, so I'll have to figure out if it's possible to make one for a case like this.
(In reply to Sam Weinig from comment #2) > But, if we can use a set instead, we could take advantage of the fact that > the JSCustomGetterFunction already stores the PropertyName and GetValueFunc. > And thus, rather than increase size, we could actually make these a bit > smaller, since the buckets would now just be Weak<JSCustomGetterFunction>. Oh, thanks, makes total sense. Wouldn't that slow down the value lookup to O(N)? If so, I guess it's tolerable for Object.getOwnPropertyDescriptor().
(In reply to Alexey Shvayka from comment #3) > (In reply to Sam Weinig from comment #2) > > But, if we can use a set instead, we could take advantage of the fact that > > the JSCustomGetterFunction already stores the PropertyName and GetValueFunc. > > And thus, rather than increase size, we could actually make these a bit > > smaller, since the buckets would now just be Weak<JSCustomGetterFunction>. > > Oh, thanks, makes total sense. > > Wouldn't that slow down the value lookup to O(N)? If so, I guess it's > tolerable for Object.getOwnPropertyDescriptor(). I don't think so, but perhaps you are thinking of something I have missed. It should still be O(1). It's essentially just going from HashMap<GetValueFunc, Weak<JSCustomGetterFunction>> to HashSet<Weak<JSCustomGetterFunction>, CustomHashTraits> The question is, can I figure out how to make that work (I am not convinced I can).
(In reply to Sam Weinig from comment #4) > I don't think so, but perhaps you are thinking of something I have missed. > It should still be O(1). It's essentially just going from To preserve identity, JSObject::getOwnPropertyDescriptor() tries to lookup already created JSCustomGetterFunction by given GetValueFunc. I might be totally missing something obvious, but is it possible to do that quickly with HashSet<Weak<JSCustomGetterFunction>, CustomHashTraits>, without iterating all the entries?
(In reply to Alexey Shvayka from comment #5) > I might be totally missing something obvious, but is it possible to do that > quickly with HashSet<Weak<JSCustomGetterFunction>, CustomHashTraits>, > without iterating all the entries? Oh, I see, we can use CustomHashTraits for that.
(In reply to Sam Weinig from comment #4) > HashSet<Weak<JSCustomGetterFunction>, CustomHashTraits> > > > The question is, can I figure out how to make that work (I am not convinced > I can). Yeah, we could also use this approach to replace WeakGCMaps that were introduced in r274528; their std::pair<> keys are stored unnecessarily.
An amusing list of all the classes that are not really what I want here: JSC::WeakSet JSC::WeakMapImpl JSC::WeakGCMap JSC::JSWeakSet JSC::JSWeakMap WTF::WeakHashSet
Note that JITThunks is doing this for NativeExecutable.
Created attachment 424459 [details] Patch
Ok, here is an initial shot at this, let's see how badly this breaks things.
(In reply to Sam Weinig from comment #11) > Ok, here is an initial shot at this, let's see how badly this breaks things. Looks like reinserting on hash table expand is asserting. I'm guessing I didn't get WeakCustomGetterOrSetterHash::equal quite right.
(In reply to Sam Weinig from comment #12) > (In reply to Sam Weinig from comment #11) > > Ok, here is an initial shot at this, let's see how badly this breaks things. > > Looks like reinserting on hash table expand is asserting. I'm guessing I > didn't get WeakCustomGetterOrSetterHash::equal quite right. Interesting, there is generic support in WTF::HashTable added for WeakPtr, which allows removing reaped weak pointers on expand. Seems useful, but not needed since we have the pruning.
Created attachment 424478 [details] Patch
Created attachment 424479 [details] Patch
Comment on attachment 424479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424479&action=review This patch looks great! Really appreciate your detailed ChangeLogs. I would defer r+ to a reviewer more knowledgeable on Source/WTF though. > Source/JavaScriptCore/runtime/WeakGCSet.h:74 > + return result.iterator->get(); WeakGCMap::ensureValue() has a null check here; without it, a few LayoutTests were crashing (https://bugs.webkit.org/show_bug.cgi?id=222739#c14). r274911 fixed functor not to invoke GC, but did it eliminate the possibility of returning a zombie (non-pruned) value here? If so, should we add an ASSERT here and, maybe, remove the null check from WeakGCMap::ensureValue()
Created attachment 424499 [details] Patch
(In reply to Sam Weinig from comment #17) > Created attachment 424499 [details] > Patch Added a version that utilizes the HashTraits::isReleasedWeakValue() function ensure released weak values don't survive rehashes.
(In reply to Alexey Shvayka from comment #16) > Comment on attachment 424479 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424479&action=review > > This patch looks great! Really appreciate your detailed ChangeLogs. I would > defer r+ to a reviewer more knowledgeable on Source/WTF though. > > > Source/JavaScriptCore/runtime/WeakGCSet.h:74 > > + return result.iterator->get(); > > WeakGCMap::ensureValue() has a null check here; without it, a few > LayoutTests were crashing > (https://bugs.webkit.org/show_bug.cgi?id=222739#c14). > r274911 fixed functor not to invoke GC, but did it eliminate the possibility > of returning a zombie (non-pruned) value here? > If so, should we add an ASSERT here and, maybe, remove the null check from > WeakGCMap::ensureValue() I think that only matters for the map version, since the key can still match if the weak value has been released. In the set version, that can't happen as the key is the value, and therefor if it got released, it can't possibly match.
Created attachment 424505 [details] Patch
<rdar://problem/75980689>
Comment on attachment 424505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424505&action=review Nice. r=me > Source/JavaScriptCore/runtime/JSObject.cpp:3528 > + using Key = std::tuple<PropertyName, typename T::CustomFunctionPointer>; nit: std::pair might be a smidge nicer? > Source/JavaScriptCore/runtime/WeakGCSet.h:36 > +template<typename T> struct WeakGCSetHashTraits : HashTraits<Weak<T>> { style nit: Maybe "template<typename T>" on its own line? > Source/WTF/ChangeLog:8 > + Adds a heterogenous HashSet::ensure, which allows lazy construction of the value to nice!
Created attachment 424690 [details] Patch
Created attachment 424694 [details] Patch
idlharness.js uses Object.getOwnPropertyDescriptor() to tests getter / setter presence, and it's also heavy on memory. Can this flaky failures (not timeouts) be related to the change? imported/w3c/web-platform-tests/xhr/idlharness.any.worker.html: https://ews-build.webkit.org/#/builders/56/builds/4449 https://ews-build.webkit.org/#/builders/56/builds/4471 https://ews-build.webkit.org/#/builders/56/builds/4475 imported/w3c/web-platform-tests/FileAPI/idlharness.worker.html: https://ews-build.webkit.org/#/builders/56/builds/4471 Should we run these tests locally with --slowPathAllocsBetweenGCs=10 JSC option and --repeat-each=100 to ensure that all descriptors are correctly reported?
Eek, good catch. Seems possible.
Any idea how to see those failures? I can no longer understand the results website well enough to figure out where to find them.
(In reply to Sam Weinig from comment #27) > Any idea how to see those failures? I can no longer understand the results > website well enough to figure out where to find them. According to the log, EWS does create pretty diffs of flaky failures but doesn't upload them if there no confirmed regressions.
(In reply to Alexey Shvayka from comment #28) > (In reply to Sam Weinig from comment #27) > > Any idea how to see those failures? I can no longer understand the results > > website well enough to figure out where to find them. > > According to the log, EWS does create pretty diffs of flaky failures but > doesn't upload them if there no confirmed regressions. Ok, will rebuild locally and see if I can repro.
(In reply to Alexey Shvayka from comment #25) > idlharness.js uses Object.getOwnPropertyDescriptor() to tests getter / > setter presence, and it's also heavy on memory. Can this flaky failures (not > timeouts) be related to the change? > > imported/w3c/web-platform-tests/xhr/idlharness.any.worker.html: > > https://ews-build.webkit.org/#/builders/56/builds/4449 > https://ews-build.webkit.org/#/builders/56/builds/4471 > https://ews-build.webkit.org/#/builders/56/builds/4475 > > imported/w3c/web-platform-tests/FileAPI/idlharness.worker.html: > > https://ews-build.webkit.org/#/builders/56/builds/4471 > > Should we run these tests locally with --slowPathAllocsBetweenGCs=10 JSC > option and --repeat-each=100 to ensure that all descriptors are correctly > reported? What does one do to get --slowPathAllocsBetweenGCs=10 in WTR? I could just set it in code and recompile, but if there is a way to enable it from the command line, that would be great (I don't see it in run-webkit-tests --help).
(In reply to Sam Weinig from comment #29) > (In reply to Alexey Shvayka from comment #28) > > (In reply to Sam Weinig from comment #27) > > > Any idea how to see those failures? I can no longer understand the results > > > website well enough to figure out where to find them. > > > > According to the log, EWS does create pretty diffs of flaky failures but > > doesn't upload them if there no confirmed regressions. > > Ok, will rebuild locally and see if I can repro. I'm not seeing it locally, so I am just going to land this and see if shows up. Might have been unrelated.
Committed r275261: <https://commits.webkit.org/r275261> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424694 [details].