417999 - JS_ArenaFreeAllocation considered excessive (harmful if not dangerous)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Crowder]

Attachment: 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).

Comment 1

[Igor Bukanov]

Comment on attachment 303784 [details] [diff] [review]
slash and burn

Patch with only minuses deserves ++ ;)

Comment 2

[Brian Crowder]

Comment on attachment 303784 [details] [diff] [review]
slash and burn

Easy code-removal.

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 303784 [details] [diff] [review]
slash and burn

Need more code removal.

/be

Comment 5

[Brian Crowder]

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.

Comment 6

[Brian Crowder]

Well, the revert idea from comment #5 is obviously bogus, I've looked over the changelog more carefully.  Retracted.

Comment 7

[Brian Crowder]

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?

Comment 8

[Brian Crowder]

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."

Comment 9

[Brian Crowder]

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?

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Brian Crowder]

jsarena.c: 3.38
jsarena.h: 3.29