614529 - Sampler callback piggy backs on sampleCheck but sampleCheck has to be side effect free

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Profiler, defect, P2)

Description

[Lars T Hansen]

For example, IncrementalMark calls SAMPLE_CHECK, but the Callback test case shows (on PPC-32, in this case) that SAMPLE_CHECK can invoke jitted code once it reaches sampleSpaceCheck.  That violates fundamental assumptions in the marker.  SAMPLE_CHECK is used extensively within the GC; every place it is used there's a danger that it violates MMgc assumptions.

An initial approximation will be to just remove all instances of SAMPLE_CHECK from the GC, but something better is presumably desirable.

Comment 1

[Lars T Hansen]

Attachment: Stopgap fix - disable sampler in mmgc code

Comment 2

[Lars T Hansen]

Attachment: Stopgap fix - disable sampler in mmgc code

Somewhat refined - this compiles on various platforms.

This will fix the assert that Brent observed.  It may however introduce a failure in as3/sampler/Callback.abc in that the test may fail because the sampler is not called enough.  C'est la guerre.

Comment 3

[Tommy Reilly]

Nice! The sampler callback feature has been a source of hard to repro bugs since it went in

Comment 4

[Tamarin Bot]

changeset: 5586:b373b194f89a
user:      Lars T Hansen <[e-mail]>
summary:   Bug 614529 - Sampler architecture can invoke arbitrary code but is used from within GC primitives: temporary workaround, disable sampler calls from MMgc and also disable affected test cases (r=lhansen, onHookForProperFix=treilly)

http://hg.mozilla.org/tamarin-redux/rev/b373b194f89a

Comment 5

[Lars T Hansen]

Pushed as temporary fix.  Also disabled Callback.as in testconfig.txt, don't forget to uncomment when a full fix is available.

Comment 6

[Tommy Reilly]

we should probably audit all uses of SAMPLE_CHECK in tamarin, specifically I worry about screwing up the jit/interpreter.

Comment 7

[Tommy Reilly]

we need to decouple the callback invocation from sampleCheck, sampleCheck is also called by the VM/JIT in various places and it must be side affect free.

Comment 8

[Tommy Reilly]

Note that we the player isn't using the sampler callback feature (yet).     An good temp measure may be to disable the sampler callback and exclude its tests for now.

Comment 9

[Edwin Smith]

Okay, two points about side effects:

1. Whenever the VM and JIT call into profiling api's, those api's must have some guarantee of being side effect free.  for example if the JIT is running on a background thread, then those calls must be legitimate.

2. Intrinsics that the JIT thinks are side-effect-free, must remain so when profiler is active.  For example, the JIT removes unnecessary calls to doubleToAtom(), which can allocate memory.  If allocating memory has visible side effects that must be preserved then we must disable said optimization.  Which would be horrible.

If AS3-based sampler code is running in the same VM instance as the tested code, then I don't see how it can ever be side effect free -- so therefore we have to ensure its only invoked from contexts where its side effects are harmless.  I think this means never calling AS3 sampler code when there is any application AS3 on the call stack.

Comment 10

[Tommy Reilly]

The sampler callback feature is a new feature that hasn't been adequately designed/tested, its not in use. 

The sampler works by recording the stack (SAMPLE_CHECK) and pushing a new stack frame (SAMPLE_FRAME).  I'm pretty sure both are side affect free though its probably worth checking.   Basically its just reading the avmcore callstack and recording pointers to a huge section of memory allocated directly from the OS (it never grows).   SAMPLE_FRAME creates a CallStackNode on the stack and pushes it on the AvmCore's call stack list, that's not a side affect to my knowledge.

The sampler as it was originally designed and as its currently used puts the onus on the sampling agent to process the samples (ie via a flash timer callback).

Comment 11

[Lars T Hansen]

(In reply to comment #10)
> The sampler callback feature is a new feature that hasn't been adequately
> designed/tested, its not in use. 

If it's not in use, then who is the intended user?

Comment 12

[Tommy Reilly]

The intended user is Flash Builder but they haven't started using it yet.

Comment 13

[Tommy Reilly]

Attachment: More surgical stopgap

Comment 14

[Lars T Hansen]

Comment on attachment 495828 [details] [diff] [review]
More surgical stopgap

OK to the best of my knowledge but note I made the same change in ZCT.cpp, needs fixin'.

Comment 15

[Tamarin Bot]

changeset: 5644:90d713af00a9
user:      Tommy Reilly <[e-mail]>
summary:   Replace coarse stopgap fix with more surgical fix no one will notice for bug 614529 (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/90d713af00a9

Comment 16

[Tommy Reilly]

Attachment: sampler death march

simple test to see sampler boundaries

Comment 17

[Tommy Reilly]

Thomas-Reillys-Mac-Pro:~ treilly$ ~/builds/tamarin-redux/release-debugger/shell/avmshell sampler_death_march.abc 
Buffer exhausted in 15.124s and 1900000 objects
Thomas-Reillys-Mac-Pro:~ treilly$ ~/builds/tamarin-redux/release-debugger64/shell/avmshell sampler_death_march.abc 
Buffer exhausted in 3.583s and 1400000 objects

Interesting, 3.5s and 1.4 million objects for 64 bit!  That matches ancedotal complaints from flex builder folks.

Comment 18

[Tommy Reilly]

Suggested fix, invoke takeSampleSafe from JIT/interp and use takeSample from SAMPLE_CHECK.  So the only time we'll ever invoke the callback is from running code (specifically function entry and backwards branches).  Its not perfect as the sample callback can still change things and cause undefined behavior is should be safe from a crashing perspective.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.