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

RESOLVED WONTFIX

Status

P2
major
RESOLVED WONTFIX
8 years ago
8 days ago

People

(Reporter: lhansen, Unassigned)

Tracking

unspecified
Q2 12 - Cyril
x86
All
Bug Flags:
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug +

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

8 years ago
Created attachment 492974 [details] [diff] [review]
Stopgap fix - disable sampler in mmgc code
(Reporter)

Comment 2

8 years ago
Created attachment 492986 [details] [diff] [review]
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.
Attachment #492974 - Attachment is obsolete: true

Comment 3

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

Comment 4

8 years ago
changeset: 5586:b373b194f89a
user:      Lars T Hansen <lhansen@adobe.com>
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
(Reporter)

Comment 5

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

Comment 6

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

Updated

8 years ago
Summary: Sampler architecture can invoke arbitrary code but is used from within GC primitives → Sampler callback piggy backs on sampleCheck but sampleCheck has to be side affect free

Comment 7

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

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

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

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

Comment 11

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

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

Comment 13

8 years ago
Created attachment 495828 [details] [diff] [review]
More surgical stopgap
Attachment #495828 - Flags: review?(lhansen)
(Reporter)

Comment 14

8 years ago
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'.
Attachment #495828 - Flags: review?(lhansen) → review+

Comment 15

8 years ago
changeset: 5644:90d713af00a9
user:      Tommy Reilly <treilly@adobe.com>
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

Updated

8 years ago
Flags: flashplayer-bug+

Updated

8 years ago
Assignee: treilly → nobody
Component: Tools → Profiler
QA Contact: tools → profiler

Updated

8 years ago
Summary: Sampler callback piggy backs on sampleCheck but sampleCheck has to be side affect free → Sampler callback piggy backs on sampleCheck but sampleCheck has to be side effect free

Updated

8 years ago
Target Milestone: Q3 11 - Serrano → Q1 12 - Brannan

Updated

8 years ago
Blocks: 603814

Comment 16

7 years ago
Created attachment 548470 [details]
sampler death march

simple test to see sampler boundaries

Comment 17

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

7 years ago
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.

Updated

7 years ago
Flags: flashplayer-qrb+
Priority: P1 → P2
Target Milestone: Q1 12 - Brannan → Q2 12 - Cyril

Updated

7 years ago
Flags: flashplayer-injection-
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
Status: NEW → RESOLVED
Last Resolved: 8 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.