The NewObjectCache is a simple mechanism that allows us to avoid hitting the initialShapeTable and ObjectGroupCompartment::NewTable for matching object creations. This is already done implicitly in the JITs via templateObjects, so this is only useful for pure-interpreter workloads. I think it is only hot in Sunspider and maybe not even there. In Bug 1211795 comment 10, Boris found that it is adding several hundred microseconds to minor collections on OSX because of the memset to sweep it. It is time for us to re-measure its effectiveness and cull it if it is not carrying its weight.

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=a410c6fc3f79 10 files changed, 3 insertions(+), 408 deletions(-) And that's a bit of an understatement, given that I haven't removed Runtime-inl.h outright yet.

This shows no performance change from m-i, so looks like this can move forward. \o/

10 files changed, 7 insertions(+), 424 deletions(-)

Comment on attachment 8716329 [details] [diff] [review] remove_NewObjectCache-v0.diff Review of attachment 8716329 [details] [diff] [review]: ----------------------------------------------------------------- Great! r++++, would review again. ::: js/src/jsobj.cpp @@ +683,2 @@ > bool > js::NewObjectWithTaggedProtoIsCachable(ExclusiveContext* cxArg, Handle<TaggedProto> proto, Nit: I think this one can also be removed.

Oooh, good catch. I think I missed it because it wasn't static -- exported, but appears to not be used elsewhere. Maybe an artifact from when half of this was in jsobjinlines.h? I'll push as soon as the try run starts showing up green. I just want to make sure that ripping out Runtime-inl.h didn't anger our build system.

Ugh. The cache-hit path was not checking for OOMAfterAllocations, so this changes the position of all of our triggered OOMs. This is going to require some OOM fixing before it can land. Will put patches here as I work through the issues, since it will all need to land at once.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2809f0ea2e666c8a86dc23befc031b413bf775 Bug 1245965 - Fix and OOM handling failure in NewMemInfoObject; r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/3e21107d9b43d2b902ff266ef4b55cef35fe8859 Bug 1245965 - Fix an OOM in ObjectGroup::newPlainObject; r=till

https://hg.mozilla.org/mozilla-central/rev/4b2809f0ea2e https://hg.mozilla.org/mozilla-central/rev/3e21107d9b43

Does this need to remain open?

(In reply to :Ms2ger from comment #13) > Does this need to remain open? Yeah, the existing landings are for bugs that removing the NOC exposed. I did not land the patch itself because there were some timeouts on try that I suspected were caused by removal of the cache. I've been meaning to retry that and see what it looks like now that Try is generally greener.

Terrence is this still on your radar? I also have to hack this code for bug 1295967. Regarding comment 0, if we need to keep the cache, we could add an |uint64_t generation;| to the table and to each entry, and purge() can just increment the table's generation. Entries in this table are pretty big anyway, so the uint64_t won't make much of a difference.

(In reply to Jan de Mooij [:jandem] from comment #20) > Terrence is this still on your radar? I also have to hack this code for bug > 1295967. Distressingly, if we remove the NewObjectCache, most of the tests on TreeHerder appear to time out. I don't really think it's that plausible though and I'd like to investigate what's really going on to cause the red before I close the bug. It's pretty low on my list right now though, so feel free to steal it. > Regarding comment 0, if we need to keep the cache, we could add an |uint64_t > generation;| to the table and to each entry, and purge() can just increment > the table's generation. Entries in this table are pretty big anyway, so the > uint64_t won't make much of a difference. That's a good idea!

I'll try to rebase this patch and see if it still causes test failures. The cache is at least 7 KB on 64-bit for each context. It's not huge, but combined with the measurable impact on all (minor) GCs it's worth removing I think.

Updated patch. There were some CGC timeouts on Try, I added these tests to the list of tests that are slow with CGC. These tests allocate many thousands of objects and this used to hit the cache, but with the CGC zeal mode we trigger a CGC for each of these allocations, so the tests become much slower.

Comment on attachment 8785253 [details] [diff] [review] Rebased patch Review of attachment 8785253 [details] [diff] [review]: ----------------------------------------------------------------- \o/

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ccbded01c01 Remove the NewObjectCache. r=terrence

FYI, the patch regressed some tests on AWFY, most of them from Kraken.

(In reply to Guilherme Lima from comment #26) > FYI, the patch regressed some tests on AWFY, most of them from Kraken. Yeah, slightly more than I expected (also some 'assorted' tests) so I backed this out for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5768dbae15 I'd be happy to look into this more and come up with a better strategy, but I don't have the time right now. I'm also torn because "Would we accept these 450 lines nowadays given the wins that aren't that big?". Probably not. Anyway, let's take a closer look at this later, I guess.

Not working on this now and it regressed perf.