Last Comment Bug 783464 - TypeCompartment::constrainedOutputs leaks on testLeftShift.js
: TypeCompartment::constrainedOutputs leaks on testLeftShift.js
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Nicolas B. Pierron [:nbp]
: general
Mentors:
Depends on:
Blocks: 772509 784187
  Show dependency treegraph
 
Reported: 2012-08-16 22:04 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-08-23 03:43 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix inverted condition. (1.04 KB, patch)
2012-08-17 15:24 PDT, Nicolas B. Pierron [:nbp]
bhackett1024: review+
nicolas.b.pierron: checkin+
Details | Diff | Review
Fix condtion for freeing compiler output vector. (6.50 KB, patch)
2012-08-22 12:12 PDT, Nicolas B. Pierron [:nbp]
bhackett1024: review+
nicolas.b.pierron: checkin+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-08-16 22:04:06 PDT
Running jit_test.py with --valgrind I get this:

==22813==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22813==    by 0x4148C4: js_malloc (Utility.h:157)
==22813==    by 0x418AB2: JSRuntime::malloc_(unsigned long, JSContext*) (jscntxt.h:892)
==22813==    by 0x418B2F: JSContext::malloc_(unsigned long) (jscntxt.h:1350)
==22813==    by 0x4F2343: js::Vector<js::analyze::SlotValue, 0ul, js::TempAllocPolicy>* JSContext::new_<js::Vector<js::analyze::SlotValue, 0ul, js::TempAllocPolicy>, JSContext*>(JSContext*) (in /home/njn/moz/mi9/js/src/d64/shell/js)
==22813==    by 0x717D3D: js::types::AutoEnterCompilation::init(JSScript*, bool, unsigned int) (jsinferinlines.h:315)
==22813==    by 0x7240E9: js::mjit::Compiler::performCompilation() (Compiler.cpp:514)
==22813==    by 0x722C78: js::mjit::Compiler::compile() (Compiler.cpp:114)
==22813==    by 0x726322: js::mjit::CanMethodJIT(JSContext*, JSScript*, unsigned char*, bool, js::mjit::CompileRequest, js::StackFrame*) (Compiler.cpp:1008)
==22813==    by 0x50AFE2: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:1468)
==22813==    by 0x50607B: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:308)
==22813==    by 0x506E50: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:492)

In other words, the TypeCompartment::constrainedOutputs vector allocated in AutoEnterCompilation::init() isn't being freed.  There is a corresponding delete in TypeCompartment::sweepCompilerOutputs() but it's conditional and I guess it's not being hit.

Judging from |hg blame|, bug 772509 is the likely cause here.
Comment 1 Nicolas B. Pierron [:nbp] 2012-08-16 23:51:37 PDT
The only allocation site of this vector is AutoEnterCompilation::init() which set the flag preventing the sweeping of the vector in sweepCompilerOutputs because the index is still used by frozen constraints created between AutoEnterCompilation::init() and the GC cycle.  This flag is supposed to be reset by AutoEnterCompilation's destructor.

Ask me for review if you want to do it, or assigned me the bug otherwise.

Do you have a precise test case or all of them have this leak?
Comment 2 Nicolas B. Pierron [:nbp] 2012-08-16 23:53:21 PDT
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #1)
> Do you have a precise test case or all of them have this leak?

Ok, I read the bug title: testLeftShift.js
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-08-17 02:23:19 PDT
I assigned it to myself by mistake -- I'm happy for you to take it.

jit_test.py only runs three tests under Valgrind, so I suspect it will be happening on plenty of other test case.
Comment 4 Nicolas B. Pierron [:nbp] 2012-08-17 15:24:05 PDT
Created attachment 652952 [details] [diff] [review]
Fix inverted condition.

Oops.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-08-19 20:31:15 PDT
Now I'm wondering why this wasn't detected automatically.  Are we not running jit_test.py with --valgrind on Linux on TBPL?
Comment 7 Paul Wright 2012-08-20 13:21:58 PDT
May want to hold up on resolving it FIXED until it lands on M-C.
Comment 8 Nicolas B. Pierron [:nbp] 2012-08-20 13:46:34 PDT
(In reply to Paul Wright from comment #7)
> May want to hold up on resolving it FIXED until it lands on M-C.

Sorry, I am used to do so with IonMonkey, and the patch will be backout from inbound after-all.
Comment 9 Nicolas B. Pierron [:nbp] 2012-08-20 14:01:24 PDT
reverted on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/9434db5cfd47
Comment 10 Nicolas B. Pierron [:nbp] 2012-08-20 16:58:46 PDT
reverted on ionmonkey: https://hg.mozilla.org/projects/ionmonkey/rev/6b2a947da355
Comment 11 Nicolas B. Pierron [:nbp] 2012-08-22 12:12:27 PDT
Created attachment 654307 [details] [diff] [review]
Fix condtion for freeing compiler output vector.

- Fix inverted condition.
- Invalidate if we are not discarding the constraints.
Comment 12 Brian Hackett (:bhackett) 2012-08-22 13:51:16 PDT
Comment on attachment 654307 [details] [diff] [review]
Fix condtion for freeing compiler output vector.

Review of attachment 654307 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jscompartment.cpp
@@ +534,5 @@
>  JSCompartment::sweep(FreeOp *fop, bool releaseTypes)
>  {
>      {
>          gcstats::AutoPhase ap(rt->gcStats, gcstats::PHASE_SWEEP_DISCARD_CODE);
> +        discardJitCode(fop, true);

This should pass the condition under which constraints are actually being discarded below, '!activeAnalysis && !gcPreserveCode', to match the assumed semantics of the discardConstraints parameter.

::: js/src/jsinfer.cpp
@@ +5728,5 @@
>  {
>  
>      if (constrainedOutputs) {
>          bool isCompiling = compiledInfo.outputIndex != RecompileInfo::NoCompilerRunning;
> +        if (discardConstraints && !isCompiling && !compartment()->activeAnalysis) {

The above change means that discardConstraints should imply !isCompiling and !activeAnalysis, can you remove those two from the 'if' and add an assert?
Comment 13 Nicolas B. Pierron [:nbp] 2012-08-22 14:27:45 PDT
(In reply to Brian Hackett (:bhackett) from comment #12)
> ::: js/src/jsinfer.cpp
> @@ +5728,5 @@
> >  {
> >  
> >      if (constrainedOutputs) {
> >          bool isCompiling = compiledInfo.outputIndex != RecompileInfo::NoCompilerRunning;
> > +        if (discardConstraints && !isCompiling && !compartment()->activeAnalysis) {
> 
> The above change means that discardConstraints should imply !isCompiling and
> !activeAnalysis, can you remove those two from the 'if' and add an assert?

The analysis can happen while we are not compiling and an analysis is only a subset of the allocations we have during a compilation.  So I cannot remove isCompiling from the list of conditions, but I can indeed remove the activeAnalysis condition.
Comment 14 Brian Hackett (:bhackett) 2012-08-22 14:32:05 PDT
isCompiling should imply activeAnalysis, so discardConstraints should imply !isCompiling.  There is an AutoEnterTypeInference wrapping AutoEnterCompilation.  The AutoEnterCompilation should assert compartment->activeAnalysis but doesn't (can you add that, if you don't mind?)
Comment 15 Nicolas B. Pierron [:nbp] 2012-08-22 16:48:42 PDT
I applied these modifications, this seems to work fine with JIT tests so I pushed it to try to avoid latest mistake:

https://tbpl.mozilla.org/?tree=Try&rev=c4741976e9e2

I will push it to inbound/ionmonkey as soon as the try server turns green.
Comment 17 David Anderson [:dvander] 2012-08-22 22:25:15 PDT
This was landed on top of an orange tree so I've backed out as:

https://hg.mozilla.org/projects/ionmonkey/rev/fbba6ea2b076

As this landed on m-i, we can take this in the next merge or re-land when the tree is green.
Comment 18 Ed Morley [:emorley] 2012-08-23 03:43:51 PDT
https://hg.mozilla.org/mozilla-central/rev/bbf6a7e1598d

Note You need to log in before you can comment on or make changes to this bug.