648106 - Allow JSContext::free to be called only during finalization.

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently we allow to call JSContext::free outside the finalization phase to support the callers that use the method to release temporary memory. This implies that the implementation must take an extra branch to check for the background finalization thread. We should stop that and insist that those places should be switched to use Foreground::free_

Comment 1

[Luke Wagner [:luke]]

Alternatively, could we use the background-free thread for calls to free outside of finalization?

Comment 2

[Nicholas Nethercote [inactive]]

Is this just to save one branch per free?  Smells like premature optimization to me.  Do you have measurements indicating it'll be noticeable?

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Is this just to save one branch per free? 

This is about saving one branch per adding the pointer to the background free set. It should not affect the time the background thread takes to call the free.

> Smells like premature optimization
> to me.  Do you have measurements indicating it'll be noticeable?

I should have filed this bug 588016. Although most of the speedup there came from lesser memory bandwidth, saving a branch per finalization helped as well (the patch avoided !cx->thread branch). So there are clear benefits from doing that.

Comment 4

[Igor Bukanov]

(In reply to comment #1)
> Alternatively, could we use the background-free thread for calls to free
> outside of finalization?

When the GC adds the thing to the background thread, it knows that that the thread is waiting. It allows to simplify quite few things.

But setting aside the question about the code complexity, why would one want to delay the release temporary allocated storage when it is still in the CPU caches and when its quick release allows for lesser heap fragmentation?

Comment 5

[Luke Wagner [:luke]]

(In reply to comment #4)
Because free isn't free.  IIRC, on Mac, its quite expensive.  However, I haven't measured time spent in free outside finalization.

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> (In reply to comment #4)
> Because free isn't free.  IIRC, on Mac, its quite expensive. 

On Mac malloc has the same if not greater expensiveness. So outside the GC if the free call harms interactivity, then malloc would also be equally responsible. In those cases we better consider using a custom allocations like arena stack to avoid the overhead of malloc/free entirely.

This is outside the scope of this bug. Here I want to remove the useless cx->gcBackhround check in the JSContext::free_ both during the GC and outside it (replacing it with Foreground:: in that case) and fix the comments about the intended usage of the free methods.

Comment 7

[Igor Bukanov]

Attachment: v1

This is a work-in-progress patch where I temporary rename cx->free_ into cx->xfree_ to spot all the places that uses it and either keep the call there or replace it with Foreground::free_ when it is clear that the call is done outside the finalization.

The patch also fixes the regression from the recent allocation refactoring when the flat string chars are finalized on the foreground, not the background, due to the usage of JSRuntime::free_.

Comment 8

[BugBot [:suhaib / :marco]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.