Bug 223613 – JSGlobalObject's m_customGetterFunctionMap and m_customSetterFunctionMap should be sets, not maps, and should use both the identifier and function pointer as the key

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Sam Weinig]

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.

[Alexey Shvayka]

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

[Sam Weinig]

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

[Alexey Shvayka]

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

[Sam Weinig]

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

[Alexey Shvayka]

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

[Alexey Shvayka]

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

[Alexey Shvayka]

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

[Sam Weinig]

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

[Yusuke Suzuki]

Note that JITThunks is doing this for NativeExecutable.

[Sam Weinig]

Created attachment 424459 [details]
Patch

[Sam Weinig]

Ok, here is an initial shot at this, let's see how badly this breaks things.

[Sam Weinig]

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

[Sam Weinig]

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

[Sam Weinig]

Created attachment 424478 [details]
Patch

[Sam Weinig]

Created attachment 424479 [details]
Patch

[Alexey Shvayka]

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

[Sam Weinig]

Created attachment 424499 [details]
Patch

[Sam Weinig]

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

[Sam Weinig]

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

[Sam Weinig]

Created attachment 424505 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75980689>

[Saam Barati]

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!

[Sam Weinig]

Created attachment 424690 [details]
Patch

[Sam Weinig]

Created attachment 424694 [details]
Patch

[Alexey Shvayka]

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?

[Sam Weinig]

Eek, good catch. Seems possible.

[Sam Weinig]

Any idea how to see those failures? I can no longer understand the results website well enough to figure out where to find them.

[Alexey Shvayka]

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

[Sam Weinig]

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

[Sam Weinig]

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

[Sam Weinig]

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

[EWS]

Committed r275261: <https://commits.webkit.org/r275261>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424694 [details].