Closed Bug 559408 Opened 10 years ago Closed 10 years ago

Turn arena pool macros into methods

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cdleary, Assigned: cdleary)

Details

Attachments

(1 file, 7 obsolete files)

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.
Attached patch Arena cleanup. (obsolete) — Splinter Review
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.
Attached patch Arena stats. (obsolete) — Splinter Review
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.
Attachment #440061 - Attachment is obsolete: true
Attachment #440360 - Attachment is obsolete: true
Attachment #440633 - Flags: review?(gal)
Passes on try server. Andreas, care to purge the beastly macros?
Attachment #440633 - Attachment is obsolete: true
Attachment #440950 - Flags: review?(gal)
Attachment #440633 - Flags: review?(gal)
Attachment #440950 - Flags: review?(gal) → review+
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.
Attached patch Arena cleanup. (obsolete) — Splinter Review
When you say to send an email to the mailing list, you mean the js-engine newsgroup?
Attachment #440950 - Attachment is obsolete: true
Attachment #441101 - Flags: review?(gal)
Comment on attachment 441101 [details] [diff] [review]
Arena cleanup.

Yes.
Attachment #441101 - Flags: review?(gal) → review+
Attached patch Arena cleanup. (obsolete) — Splinter Review
Axes a few more macros and running on try server...
Attachment #441101 - Attachment is obsolete: true
Seems to pass on try server. How long should we wait on the newsgroup?
Attachment #441193 - Flags: review?(gal)
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.
Attachment #441193 - Flags: review?(gal)
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
Whiteboard: fixed-in-tracemonkey
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.
Attached patch Arena pool refactor. (obsolete) — Splinter Review
Re-review please.
Attachment #441193 - Attachment is obsolete: true
Attachment #456335 - Flags: review?
Forgot moz header in new inlines file. Not a lot of changes since last review, just would like a once-over.
Attachment #456335 - Attachment is obsolete: true
Attachment #456337 - Flags: review?
Attachment #456335 - Flags: review?
Attachment #456337 - Flags: review? → review?(gal)
http://hg.mozilla.org/mozilla-central/rev/ec3acdc9e4d2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #456337 - Flags: review?(gal)
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.
You need to log in before you can comment on or make changes to this bug.