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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Actually there is a bug in jstracer.h. We declare ResetJIT(cx) but it has a different signature in the implementation.

Comment 3

[Andreas Gal :gal]

Attachment: patch

I am not sure I understand why the old code links. Maybe because its extern "C"?

Comment 4

[Andreas Gal :gal]

Attachment: patch

Comment 5

[Andreas Gal :gal]

I think we call ResetJIT with too few arguments in jsregexp, so asking for b?

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Andreas Gal :gal]

Waiting for bent to give this a try, and then we will need review from Brendan for the public API change.

Comment 8

[David Mandelin [:dmandelin]]

Comment on attachment 414619 [details] [diff] [review]
patch

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?

Comment 9

[Andreas Gal :gal]

The ResetJIT one is a macro to ResetJITImpl. I don't much like that trace viz hackery in there either.

Comment 10

[Mike Shaver (:shaver emeritus)]

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.

Comment 11

[Mike Shaver (:shaver emeritus)]

(Is a physical thread a hardware thread, then, vs. an OS thread?  Are we really tying up a pipe with a suspended worker? Oy.)

Comment 12

[Andreas Gal :gal]

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.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Renom'ing based on comment 12.

Comment 14

[Damon Sicore (:damons)]

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

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 16

[Andreas Gal :gal]

Comment on attachment 414619 [details] [diff] [review]
patch

API change, so asking for SR.

Comment 17

[Robert Sayre]

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 18

[Brendan Eich [:brendan]]

Comment on attachment 414619 [details] [diff] [review]
patch

> /*
>+ * 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.

..12345678901234567890123456789012345678901234567890123456789012345678901234567890

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.

/be

Comment 19

[(not currently active) Ted Mielczarek]

It looks like this is causing static analysis bustage:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1261397146.1261397470.25630.gz
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

Comment 20

[Benjamin Smedberg]

Attachment: ResetJIT is in fact safe to call on trace (?), rev. 1

jorendorff/gal are apparently OOTO, trying dvander/brendan.

Comment 21

[Benjamin Smedberg]

Backed out: http://hg.mozilla.org/mozilla-central/rev/db08d5352918

Comment 22

[Robert Sayre]

oops, sorry I didn't see that.

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 418629 [details] [diff] [review]
ResetJIT is in fact safe to call on trace (?), rev. 1

Looks safe.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob: is this planned to come across to 1.9.2 anytime soon? Has it already?

Comment 25

[Robert Sayre]

pushed with bsmedberg's analysis fix:

http://hg.mozilla.org/mozilla-central/rev/d13ed0204332

Comment 26

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4f67926154dc