Bug 177909 – Make sure all prototypes under poly proto get added into the VM's prototype map

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

[Saam Barati]

... But they should. This is an invariant I broke.

[Saam Barati]

Created attachment 322758 [details]
patch

[Keith Miller]

Comment on attachment 322758 [details]
patch

r=me if you fix the build issues.

[Keith Miller]

Comment on attachment 322758 [details]
patch

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:242
>          size_t inlineCapacity = pc[3].u.operand;

rs=me on adding struct offset names.

[Saam Barati]

Created attachment 322765 [details]
patch for landing

[Saam Barati]

Created attachment 322766 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 322766 [details]
patch for landing

Clearing flags on attachment: 322766

Committed r222901: <http://trac.webkit.org/changeset/222901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34829138>

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #6)
> Comment on attachment 322766 [details]
> patch for landing
> 
> Clearing flags on attachment: 322766
> 
> Committed r222901: <http://trac.webkit.org/changeset/222901>
The following two tests started timing out on Release JSC bots after this change:
microbenchmarks/direct-construct-arity-mismatch.js.ftl-eager
microbenchmarks/direct-construct.js.ftl-eager

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20JSC%20%28Tests%29/builds/187

[Saam Barati]

(In reply to Ryan Haddad from comment #9)
> (In reply to WebKit Commit Bot from comment #6)
> > Comment on attachment 322766 [details]
> > patch for landing
> > 
> > Clearing flags on attachment: 322766
> > 
> > Committed r222901: <http://trac.webkit.org/changeset/222901>
> The following two tests started timing out on Release JSC bots after this
> change:
> microbenchmarks/direct-construct-arity-mismatch.js.ftl-eager
> microbenchmarks/direct-construct.js.ftl-eager
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20JSC%20%28Tests%29/builds/187

Will look into it

[Saam Barati]

(In reply to Ryan Haddad from comment #9)
> (In reply to WebKit Commit Bot from comment #6)
> > Comment on attachment 322766 [details]
> > patch for landing
> > 
> > Clearing flags on attachment: 322766
> > 
> > Committed r222901: <http://trac.webkit.org/changeset/222901>
> The following two tests started timing out on Release JSC bots after this
> change:
> microbenchmarks/direct-construct-arity-mismatch.js.ftl-eager
> microbenchmarks/direct-construct.js.ftl-eager
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20JSC%20%28Tests%29/builds/187

Will fix this in:
https://bugs.webkit.org/show_bug.cgi?id=177952

[Filip Pizlo]

Comment on attachment 322766 [details]
patch for landing

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:253
> +        if (structure->hasPolyProto()) {
> +            JSObject* prototype = constructor->prototypeForConstruction(vm, exec);
> +            result->putDirect(vm, structure->polyProtoOffset(), prototype);
> +            vm.prototypeMap.addPrototype(prototype);
> +        }

I don't think that this was a good idea.

This means that if we're using poly proto aggressively (like 1:1 mapping of object and prototype), we'll have this ginormous HashSet of Weak references hanging around.  If we really need every prototype to be tracked, then it seems a lot better to just use a bit in the object header, instead of growing a ginormous HashSet.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #12)
> Comment on attachment 322766 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322766&action=review
> 
> > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:253
> > +        if (structure->hasPolyProto()) {
> > +            JSObject* prototype = constructor->prototypeForConstruction(vm, exec);
> > +            result->putDirect(vm, structure->polyProtoOffset(), prototype);
> > +            vm.prototypeMap.addPrototype(prototype);
> > +        }
> 
> I don't think that this was a good idea.
> 
> This means that if we're using poly proto aggressively (like 1:1 mapping of
> object and prototype), we'll have this ginormous HashSet of Weak references
> hanging around.  If we really need every prototype to be tracked, then it
> seems a lot better to just use a bit in the object header, instead of
> growing a ginormous HashSet.

Anyway, I'm tracking fixing this in: https://bugs.webkit.org/show_bug.cgi?id=178051

[Ryosuke Niwa]

Looks like this was ~1% regression on Speedometer 2's react test case?
https://perf.webkit.org/v3/#/charts?since=1505283360823&zoom=(1506767770443.9912-1507709310260.7344)&paneList=((18-966-28983529-null-(5-2.5-500)))&repository=1

[Saam Barati]

(In reply to Ryosuke Niwa from comment #14)
> Looks like this was ~1% regression on Speedometer 2's react test case?
> https://perf.webkit.org/v3/#/charts?since=1505283360823&zoom=(1506767770443.
> 9912-1507709310260.7344)&paneList=((18-966-28983529-null-(5-2.5-
> 500)))&repository=1

This code is no longer in the tree.

[Ryosuke Niwa]

(In reply to Saam Barati from comment #15)
> (In reply to Ryosuke Niwa from comment #14)
> > Looks like this was ~1% regression on Speedometer 2's react test case?
> > https://perf.webkit.org/v3/#/charts?since=1505283360823&zoom=(1506767770443.
> > 9912-1507709310260.7344)&paneList=((18-966-28983529-null-(5-2.5-
> > 500)))&repository=1
> 
> This code is no longer in the tree.

Hm... it looks like we never recovered from this :( Maybe there are other regressions looming around.

https://perf.webkit.org/v3/#/charts?since=1505283360823&paneList=((18-966-(1506582270543.6445-1508505237448.509)-null-(5-2.5-500)))&repository=1

[Saam Barati]

(In reply to Ryosuke Niwa from comment #16)
> (In reply to Saam Barati from comment #15)
> > (In reply to Ryosuke Niwa from comment #14)
> > > Looks like this was ~1% regression on Speedometer 2's react test case?
> > > https://perf.webkit.org/v3/#/charts?since=1505283360823&zoom=(1506767770443.
> > > 9912-1507709310260.7344)&paneList=((18-966-28983529-null-(5-2.5-
> > > 500)))&repository=1
> > 
> > This code is no longer in the tree.
> 
> Hm... it looks like we never recovered from this :( Maybe there are other
> regressions looming around.
> 
> https://perf.webkit.org/v3/#/charts?since=1505283360823&paneList=((18-966-
> (1506582270543.6445-1508505237448.509)-null-(5-2.5-500)))&repository=1

Yeah I think it’s unlikely this is the reason. My original patch was a regression because it forced us to allocate a weak handle and put something in a hashmap. The code in ToT does a CAS on the slow path, and the fast path is just a load and branch.