600153 - TM: fix storeAccSet for functionProbe()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

functionProbe() has this CallInfo:

  JS_DEFINE_CALLINFO_3(static, BOOL, functionProbe, CONTEXT, FUNCTION, BOOL, 0, 0)

The last arg shouldn't be '0'.  It should probably be ACCSET_STORE_ANY.  This means that functionProbe can safely write to any part of memory that LIR can access.   If functionProbe doesn't write any part of memory that LIR can access, the last argument should be ACCSET_NONE.  Given what functionProbe looks like -- it calls a callback that does who-knows-what -- I'd be surprised if this were the case... when in doubt, ACCSET_STORE_ANY is certainly the safe option.  See the big comment under "Aliasing" in nanojit/LIR.cpp for more details about AccSets.

Steve, can I ask how/why you put '0' as the last argument here?  Every other CallInfo I can see uses ACCSET_<something>.  I ask because if you get that value wrong it can lead to very subtle bugs where the CSE pass combines things that it shouldn't, and the thought of people getting it wrong is something that scares me and I'd like to find ways to make it harder to get wrong in the future.  Thanks!

Comment 1

[Steve Fink [:sfink] [:s:]]

Hm. I'd like to be able to say ACCSET_NONE, but I already have an example usage that I think would need ACCSET_OTHER (it logs calls by appending them to a big nasty JS Array). It also reads script->filename and the name of the function (an atom); would that throw it over to ACCSET_LOAD_ANY (aka ACCSET_ALL)?

Philosophically, I want functionProbe to perturb what code gets executed as little as possible when it is enabled (as in, when a function callback is registered.) When no callback is registered, no functionProbe invocation will be emitted, so that's fine regardless of the accSet. When a callback is registered, performance will suck no matter what accSet is set to, so I'm not concerned as much with the performance impact. But it would be nice if it was guaranteed that the same number of pure function calls happened. It's not critical, though, so it's probably safest to just do ACCSET_STORE_ANY. (The mechanism is incomplete anyway and misses function calls on various paths already, so an aggressive and unsafe accset just to avoid 3rd order effects doesn't make much sense right now.)

As for why the '0', I inherited that from dmandelin's original patch for this: <https://bug507012.bugzilla.mozilla.org/attachment.cgi?id=391245>, and *all* of this was completely new to me at that time so I didn't check any of it. I'm cc'ing dmandelin.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> Hm. I'd like to be able to say ACCSET_NONE, but I already have an example usage
> that I think would need ACCSET_OTHER (it logs calls by appending them to a big
> nasty JS Array). It also reads script->filename and the name of the function
> (an atom); would that throw it over to ACCSET_LOAD_ANY (aka ACCSET_ALL)?

It would be ACCSET_STORE_ANY, which is the same as ACCSET_LOAD_ANY and ACCSET_ALL, but indicates that its stores that are relevant.

> Philosophically, I want functionProbe to perturb what code gets executed as
> little as possible when it is enabled (as in, when a function callback is
> registered.)

Yeah, but if you insert an impure function call in the middle of the instruction stream it cannot help but perturb some optimizations... I don't see a way around this, sorry.

> When a callback is
> registered, performance will suck no matter what accSet is set to, so I'm not
> concerned as much with the performance impact. But it would be nice if it was
> guaranteed that the same number of pure function calls happened.

I didn't understand that last sentence... are you talking about C++ functions or JS functions?

> As for why the '0', I inherited that from dmandelin's original patch for this:
> <https://bug507012.bugzilla.mozilla.org/attachment.cgi?id=391245>, and *all* of
> this was completely new to me at that time so I didn't check any of it.

Ah, ok.  Hopefully that won't happen again...

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 54611:10c9ffdbc59b)

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 479239 [details] [diff] [review]
patch (against TM 54611:10c9ffdbc59b)

(In reply to comment #2)
> (In reply to comment #1)
> > Hm. I'd like to be able to say ACCSET_NONE, but I already have an example usage
> > that I think would need ACCSET_OTHER (it logs calls by appending them to a big
> > nasty JS Array). It also reads script->filename and the name of the function
> > (an atom); would that throw it over to ACCSET_LOAD_ANY (aka ACCSET_ALL)?
> 
> It would be ACCSET_STORE_ANY, which is the same as ACCSET_LOAD_ANY and
> ACCSET_ALL, but indicates that its stores that are relevant.

Right. But to clarify: does access to script->filename and the name of the function require ACCSET_*_ANY, or just ACCSET_OTHER? I'd be willing to restrict users from accessing either the return value or the stack.

> > Philosophically, I want functionProbe to perturb what code gets executed as
> > little as possible when it is enabled (as in, when a function callback is
> > registered.)
> 
> Yeah, but if you insert an impure function call in the middle of the
> instruction stream it cannot help but perturb some optimizations... I don't see
> a way around this, sorry.

By documenting it as only being usable for pure callbacks. If you need an impure callback, use the jsd hooks and accept that you'll disable TM. (I'm not actually arguing for that; I'm just clarifying what I was getting at.)

> > When a callback is
> > registered, performance will suck no matter what accSet is set to, so I'm not
> > concerned as much with the performance impact. But it would be nice if it was
> > guaranteed that the same number of pure function calls happened.
> 
> I didn't understand that last sentence... are you talking about C++ functions
> or JS functions?

JS functions. The purpose of the callback is for mediumweight profilers to capture the exact set of JS functions that are called. So it's ok to degrade the performance somewhat, but not the behavior any more than necessary.

So eg the jsd debugHooks prevent tracing, which impacts behavior too much. Although honestly, it's the magnitude of the performance impact that was the real reason why I couldn't use jsd, though I'd argue that the performance impact was enough to influence behavior (fewer timers would fire etc.)

None of which really matters. I'm fine with ACCSET_STORE_ANY. I'm just curious about the exact semantics of what I messed up.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> 
> Right. But to clarify: does access to script->filename and the name of the
> function require ACCSET_*_ANY, or just ACCSET_OTHER? I'd be willing to restrict
> users from accessing either the return value or the stack.

Currently there are only three access regions:  STACK, RSTACK, OTHER.  You could probably get away with just ACCSET_OTHER.

But bug 584279, which has a patch waiting for a review, changes it so we have 25 regions.  This allows more CSE to happen, and makes loop-invariant code motion possible.


> By documenting it as only being usable for pure callbacks. If you need an
> impure callback, use the jsd hooks and accept that you'll disable TM. (I'm not
> actually arguing for that; I'm just clarifying what I was getting at.)

Right... a pure callback is pretty useless though -- the only interesting thing a pure function can do is return a value and that isn't happening here, AFAICT.  A callback almost certainly has to have side-effects to be interesting.  Now it is possible that the callback may have some side-effect such as storing a value in a place that isn't visible from LIR... but it's all sounding esoteric and dangerous.

> So it's ok to degrade
> the performance somewhat, but not the behavior any more than necessary.

ACCSET_STORE_ANY should be ok then.

> None of which really matters. I'm fine with ACCSET_STORE_ANY. I'm just curious
> about the exact semantics of what I messed up.

Hopefully it's clearer now?

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/b91d6314aaae

Comment 7

[Robert Sayre]

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

Comment 8

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Ts, Cold MED Dirty Profile regression on Firefox. boo-urns :(

  Previous: avg 420.356 stddev 3.679 of 30 runs up to a9d1ad0bc386
  New     : avg 430.832 stddev 1.905 of 5 runs since a60414d076b5
  Change  : +10.475 (2.49% / z=2.847)
  Graph   : http://mzl.la/bZFaB3

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-ts_cold_generated_med] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 9

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Win7 Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2014.419 stddev 40.480 of 30 runs up to a9d1ad0bc386
  New     : avg 1901.610 stddev 12.432 of 5 runs since a60414d076b5
  Change  : -112.809 (-5.6% / z=2.787)
  Graph   : http://mzl.la/9gFu4n

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 10

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Win7 Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 127.610 stddev 4.222 of 30 runs up to a9d1ad0bc386
  New     : avg 116.384 stddev 0.751 of 5 runs since a60414d076b5
  Change  : -11.226 (-8.8% / z=2.659)
  Graph   : http://mzl.la/bZu5UP

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 11

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2045.275 stddev 49.676 of 30 runs up to a9d1ad0bc386
  New     : avg 1936.120 stddev 13.937 of 5 runs since a60414d076b5
  Change  : -109.155 (-5.34% / z=2.197)
  Graph   : http://mzl.la/b0dlou

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 12

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 129.703 stddev 4.099 of 30 runs up to a9d1ad0bc386
  New     : avg 117.954 stddev 0.660 of 5 runs since a60414d076b5
  Change  : -11.749 (-9.06% / z=2.866)
  Graph   : http://mzl.la/avZij4

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.