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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 final-fixed)

Details

(Whiteboard: [has reviewed patch])

Attachments

(2 attachments, 2 obsolete attachments)

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

Updated

9 years ago
tracking-fennec: --- → ?
Priority: -- → P2
(Assignee)

Updated

9 years ago
Assignee: general → gal

Updated

9 years ago
tracking-fennec: ? → 1.0+

Updated

8 years ago
tracking-fennec: 1.0+ → ---
(Assignee)

Comment 1

8 years ago
Created attachment 414614 [details] [diff] [review]
patch
(Assignee)

Comment 2

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

Comment 3

8 years ago
Created attachment 414617 [details] [diff]
patch

I am not sure I understand why the old code links. Maybe because its extern "C"?
Attachment #414614 - Attachment is obsolete: true
(Assignee)

Comment 4

8 years ago
Created attachment 414618 [details] [diff] [review]
patch
(Assignee)

Updated

8 years ago
Attachment #414618 - Flags: review?(dmandelin)
(Assignee)

Comment 5

8 years ago
I think we call ResetJIT with too few arguments in jsregexp, so asking for b?
Flags: blocking1.9.2?
(Assignee)

Comment 6

8 years ago
Created attachment 414619 [details] [diff] [review]
patch
Attachment #414618 - Attachment is obsolete: true
Attachment #414618 - Flags: review?(dmandelin)
(Assignee)

Comment 7

8 years ago
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]
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?
(Assignee)

Comment 9

8 years ago
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.)
(Assignee)

Comment 12

8 years ago
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+
(Assignee)

Updated

8 years ago
Attachment #414619 - Flags: review?(dvander)
Attachment #414619 - Flags: review?(dvander) → review+
(Assignee)

Comment 16

8 years ago
Comment on attachment 414619 [details] [diff] [review]
patch

API change, so asking for SR.
Attachment #414619 - Flags: superreview?(brendan)

Comment 17

8 years ago
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]
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
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

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

jorendorff/gal are apparently OOTO, trying dvander/brendan.
Attachment #418629 - Flags: review?(dvander)

Comment 22

8 years ago
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?

Comment 25

8 years ago
pushed with bsmedberg's analysis fix:

http://hg.mozilla.org/mozilla-central/rev/d13ed0204332
Status: NEW → RESOLVED
Last Resolved: 8 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.