Created attachment 303784 [details] [diff] [review] slash and burn This API foils the patch from bug 408921 comment #18 (with -DJS_ARENA_GROSS_SIZE_THRESHOLD=xxx). It's not entirely clear how or why, but discussion with Brendan seems to indicate that non-LIFO behavior in arenas is fundamentally unsound. The bug 408921 comment #18 causes multiple test failures, one of which is in tests/ecma_3/Statements/regress-324650.js, crashing on the lone JS_ArenaFreeAllocation call in js/src, which is in SpanDep code (bug 417995 has a patch to remove that usage, this patch depends upon that).
Attachment #303784 - Flags: review?(igor)
Comment on attachment 303784 [details] [diff] [review] slash and burn Patch with only minuses deserves ++ ;)
Attachment #303784 - Flags: review?(igor) → review+
Comment on attachment 303784 [details] [diff] [review] slash and burn Easy code-removal.
Let's be precise: JS_ArenaFreeAllocation is not unsound (as in p -> q but !p; invalid would be p !-> q but p), it is buggy -- noting dependency on bug 239721. The unsoundness I mentioned on IRC was the scavenging from low (near first) arenas in a pool by setting current low, which the lcc paper that inspired jsarena talked about. That can result in non-LIFO mark and disastrous release. Getting rid of code is good. The only reason I complicated arena pools with the oversized allocation junk and JS_ArenaFreeAllocation was for spandeps, and then only to conserve the mark/release of tempPool. I thought I might have other use cases, at the time. But it was a mistake as this patch shows. Please remove all the oversized allocation HEADER-back-pointer-pointer stuff too and verify that bug 239721 is fixed and mark it too, when you are done. Thanks, /be
Target Milestone: --- → mozilla1.9beta4
Comment on attachment 303784 [details] [diff] [review] slash and burn Need more code removal. /be
There's rather a lot more risk involved in removing the now-vestigial header-code than in removing the code proposed in the patch. Any chance we could do that as a separate bug? I'd rather move forward with gross-not-net and come back to this (or else just revert jsarena.c to version 3.18 or so, since it seems a lot of its history has grown around fixes to functionality introduced there or within a revision or two) later.
Well, the revert idea from comment #5 is obviously bogus, I've looked over the changelog more carefully. Retracted.
So these headers actually still serve a purpose, don't they? Or are we abandoning oversized allocations entirely? Without these headers, aren't we forced to begin a new arena, even if the arena created before the oversized arena actually still has space in it for the current allocation?
cvs log for jsarena.c:3.19 -- "To avoid searching for the previous arena, in order to update its next member upon reallocation of the arena containing a large single allocation, the oversized arena has a back-pointer to that next member stored (but not as allocable space within the arena) in a (JSArena **) footer at its end."
That log message refers to the old "footer" concept, but the current headers still serve the same purpose for oversized allocations in the middle of the arena pool, right?
Crowder: ok, let's push off the rest of the oversized arena reconsideration to bug 239721. I do think that has rotted long enough on my list though, so would appreciate your help finally killing it. /be
Status: NEW → ASSIGNED
Summary: JS_ArenaFreeAllocation considered dangerous → JS_ArenaFreeAllocation considered excessive (harmful if not dangerous)
jsarena.c: 3.38 jsarena.h: 3.29
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.