Closed Bug 474500 Opened 11 years ago Closed 10 years ago

TM: Need an API to flush and (optionally?) deallocate the code cache per thread


(Core :: JavaScript Engine, defect, P2)




Tracking Status
status1.9.2 --- final-fixed


(Reporter: bent.mozilla, Assigned: gal)



(Whiteboard: [has reviewed patch])


(2 files, 2 obsolete files)

Workers can be suspended and we currently achieve this by suspending the physical thread on which it is running. We need some way to deallocate the code cache in this case so that multiple suspended threads don't tie up a bunch of free memory. Andreas says that the cache will automatically be reallocated when we resume tracing.
tracking-fennec: --- → ?
Priority: -- → P2
Assignee: general → gal
tracking-fennec: ? → 1.0+
tracking-fennec: 1.0+ → ---
Attached patch patch (obsolete) — Splinter Review
Actually there is a bug in jstracer.h. We declare ResetJIT(cx) but it has a different signature in the implementation.
Attached patch patch (obsolete) — Splinter Review
I am not sure I understand why the old code links. Maybe because its extern "C"?
Attachment #414614 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #414618 - Flags: review?(dmandelin)
I think we call ResetJIT with too few arguments in jsregexp, so asking for b?
Flags: blocking1.9.2?
Attached patch patchSplinter Review
Attachment #414618 - Attachment is obsolete: true
Attachment #414618 - Flags: review?(dmandelin)
Waiting for bent to give this a try, and then we will need review from Brendan for the public API change.
Comment on attachment 414619 [details] [diff] [review]

So we'll have 3 functions that all do the same thing, because the JS_ one just calls the js_ one which just calls ResetJIT? Do we need all 3?
The ResetJIT one is a macro to ResetJITImpl. I don't much like that trace viz hackery in there either.
Can someone explain why this should block 1.9.2?

Some questions!

Is it a regression from 3.5?  Is it a long-term memory leak?  Does that leak persist if the user navigates away from the page? How common is it that worker threads are suspended for a long time?

I believe that the answers are "no", "no", "no", "not very", so minusing, but renom if the answers are wrong.
Flags: blocking1.9.2? → blocking1.9.2-
(Is a physical thread a hardware thread, then, vs. an OS thread?  Are we really tying up a pipe with a suspended worker? Oy.)
ResetJIT is called with a missing 2nd argument (signature mismatch in header file). I would like to fix that. bent's use of this I don't see as a 192 blocker. But since I am already in town I can as well expose the internal friend api to do what he is trying to do.
Renom'ing based on comment 12.
Flags: blocking1.9.2- → blocking1.9.2?
bent, he says he doesn't see it as a blocker.  Not sure why you renom'd, so, I'm clearing the flag to give you a chance to explain.  :)
Flags: blocking1.9.2?
(In reply to comment #14)
> I'm clearing the flag to give you a chance to explain.  :)

> (In reply to comment #12)
> > ResetJIT is called with a missing 2nd argument (signature mismatch in
> > header file). I would like to fix that.

That's pretty bad and I believe that should block.
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Attachment #414619 - Flags: review?(dvander)
Attachment #414619 - Flags: review?(dvander) → review+
Comment on attachment 414619 [details] [diff] [review]

API change, so asking for SR.
Attachment #414619 - Flags: superreview?(brendan)
This is an API addition, and this blocks so I'm going to land this today. We'll have a little time to sort it out if he objects later, since I don't see anyone calling it yet.
Comment on attachment 414619 [details] [diff] [review]

> /*
>+ * Flush the code cache for the current thread. The operation might be
>+ * delayed if the cache cannot be flushed currently because native
>+ * code is currently executing.


Your comment should be wrapped to tw=79 per prevailing style.

The comment does not define "code cache" (as opposed to other caches associated with the current thread's data in the context [or runtime if !JS_THREADSAFE]). The API name also does not say "Code":

>+ */
>+extern JS_PUBLIC_API(void)
>+JS_FlushCaches(JSContext *cx);

Please make the API have a more precise name, lest people be confused and start calling it too much.

It looks like this is causing static analysis bustage:
In file included from /builds/static-analysis-buildbot/slave/full/build/js/src/jstracer.cpp:15076:
/builds/static-analysis-buildbot/slave/full/build/js/src/jstracer.cpp: In function 'void js_FlushJITCache(JSContext*)':
/builds/static-analysis-buildbot/slave/full/build/js/src/jstracer.cpp:2299: error: cannot call JS_REQUIRES_STACK function ResetJITImpl(JSContext*)
make[4]: *** [jstracer.o] Error 1
jorendorff/gal are apparently OOTO, trying dvander/brendan.
Attachment #418629 - Flags: review?(dvander)
oops, sorry I didn't see that.
Comment on attachment 418629 [details] [diff] [review]
ResetJIT is in fact safe to call on trace (?), rev. 1

Looks safe.
Attachment #418629 - Flags: review?(dvander) → review+
Whiteboard: [has reviewed patch]
Rob: is this planned to come across to 1.9.2 anytime soon? Has it already?
pushed with bsmedberg's analysis fix:
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #414619 - Flags: superreview?(brendan)
You need to log in before you can comment on or make changes to this bug.