I've got a fever, and the only cure is more cleanup.

Checking for a dup... https://bugzilla.mozilla.org/buglist.cgi?quicksearch=jsarena Hmm, I thought we had a bug on de-uglifying this old C code with C++. Andreas may remember it. /be

I think I'm nearly done anyway -- this cleanup is a prereq if we want to try out the variable growth strategy we talked about in bug 558971.

Doesn't give any speedup on my Linux box, even with the additional inlines. Will test on OS X and profile a little bit more.

Creates a build-time option for them. Now working on a lazily-freed arena solution to see if there's a benefit over recycling.

I'm also going to submit it to the try server to make sure it doesn't blow things up.

Passes on try server. Andreas, care to purge the beastly macros?

Comment on attachment 440950 [details] [diff] [review] Arena cleanup and metering configure option. > dnl ======================================================== >+dnl Arena Metering >+dnl ======================================================== >+MOZ_ARG_ENABLE_BOOL(arena-metering, >+[ --enable-arena-metering Enable arena metering output], >+ JS_ARENAMETER=1, >+ JS_ARENAMETER= ) >+if test -n "$JS_ARENAMETER"; then >+ AC_DEFINE(JS_ARENAMETER) >+ AC_DEFINE(JS_BASIC_STATS) >+fi I would prefer an environment variable that turns it on and always compiled it when debug. Otherwise the code bit rots right away. >-JS_PUBLIC_API(void) >-JS_InitArenaPool(JSArenaPool *pool, const char *name, size_t size, >- size_t align, size_t *quotap) Maybe email to the mailing list that we retire these functions. Its a pretty terrible API, so I don't think anyone will miss it. Maybe also feedback? brendan, just to make sure he is ok with it. > #ifdef JS_ARENAMETER >- memset(&pool->stats, 0, sizeof pool->stats); >- pool->stats.name = strdup(name); >- pool->stats.next = arena_stats_list; >- arena_stats_list = &pool->stats; >+ stats.init(name, arena_stats_list); >+ arena_stats_list = &stats; > #endif #ifdef DEBUG. Seriously this stuff bitrots. Ask Gregor about the GC stuff he recently had to bring back up to snuff. >+ /* NB: always subtract nb from limit rather than adding nb to p to avoid overflowing a >+ * 32-bit address space. See bug 279273. */ /* * */ >+ void *p = (void *)a->avail; I think we usually do (void *) a->avail but I might be wrong.

When you say to send an email to the mailing list, you mean the js-engine newsgroup?

Comment on attachment 441101 [details] [diff] [review] Arena cleanup. Yes.

Axes a few more macros and running on try server...

Seems to pass on try server. How long should we wait on the newsgroup?

Comment on attachment 441193 [details] [diff] [review] Arena cleanup. This seems to cause a perf regression in parsemark -- going to look at what changed from the macro version a little more closely.

Try to diff profiles in shark. Works really well.

(In reply to comment #14) > http://hg.mozilla.org/tracemonkey/rev/919950c7f0f0 ahem, this burned the tree all night. I think this should fix it: http://hg.mozilla.org/tracemonkey/rev/e0d454817dfd

so, I had to back this patch out, and its followups. bugs: there's a strdup usage in debug metering that doesn't compile on windows. use JS_strdup. uint8_t doesn't compile on windows: use uint8 add this ifdef +#ifdef JS_ALIGN_OF_POINTER JS_ASSERT((jsuword(p) & headerBaseMask()) == 0); +#endif

(In reply to comment #16) > so, I had to back this patch out, and its followups. I saw, sorry about that... The version of this patch I tested on try server didn't have so many things inlined -- when I lifted those few things I should have retry'd, since JSArenaPool is used by code outside of JS. Lesson learned1

The alternative lesson to learn is to watch the tree after you land a patch. That way you can quickly do small fixes for minor unexpected bustage.

Is there any reason this can't be relanded?

(In reply to comment #19) > Is there any reason this can't be relanded? There was no pressing need for it and the tree bustage made me not want to work on it more. :-) Have a use case?

The existing code is supremely ugly. I buy you dinner if you land the patch. I really can't see the macro code any more.

Re-review please.

Forgot moz header in new inlines file. Not a lot of changes since last review, just would like a once-over.

This bug is marked as fixed, but AFAICT it was backed out and never got followed up. I noticed this because I was looking at the history of jsarena.cpp, because I'm about to try to simplify it (bug 675150), which will bitrot this patch horribly, but that doesn't seem to matter since it's clearly not going anywhere.