... But they should. This is an invariant I broke.
Created attachment 322758 [details] patch
Comment on attachment 322758 [details] patch r=me if you fix the build issues.
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.
Created attachment 322765 [details] patch for landing
Created attachment 322766 [details] patch for landing
Comment on attachment 322766 [details] patch for landing Clearing flags on attachment: 322766 Committed r222901: <http://trac.webkit.org/changeset/222901>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34829138>
(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
(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
(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
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.
(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
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
(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.
(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
(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.