594591 - TM: abort recording if there are too many slots

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

From bug 591631 comment 17, w.r.t. the example program in that bug:

> Having to walk 4,300 local slots every snapshot() to determine the typemap is
> sucky (academically, "total clownshoes"). We could keep track of the
> typemap incrementally as we record - which sounds tricky and error-prone
> given the current TR design. Easier might be caching the last known
> typemap and watermarking the -/+ positions on the stack, to only recompute
> a slice at the end.
>
> We'd have to be careful of opcodes like SETNAME or SETELEM which can
> change random stack slots.
>
> My guess is that test cases like this should be under the "don't bother
> tracing" category right now. If nfixed > XYZ, don't even record.

We've decided to do the latter -- abort recording if there are too many slots.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Can we detect this situation in jitprofiling and blacklist without even trying to record to start with, since the slot count should be known up front?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: profiler change

This patch updates the profiler to avoid tracing if there are too many slots used.

Comment 3

[Nicholas Nethercote [inactive]]

I think this should be a hard check instead of inside the profiler.  In other words, we should avoid tracing such fragments even if -p is not specified.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Nicholas, I think it would make sense to check both places.

Bill, I'm not sure I'm qualified to review this patch, sorry...

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> Nicholas, I think it would make sense to check both places.

That doesn't make any sense.  If the check always occurs with -j then adding another check that only runs with -j -m -p will be redundant.

Another way to explain:  we currently abort recording for lots of reasons, right?  This is just another one.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #5)
> That doesn't make any sense.  If the check always occurs with -j then adding
> another check that only runs with -j -m -p will be redundant.

Profiling is a lot cheaper than recording, so if we hit the check during profiling, we've wasted a lot less time than if we hit it during recording. So in that light, it probably does make sense to do it in both places.

Nick, could we just add an abort in monitorRecording if the number of slots in the current script is too high? Or did you have something else in mind?

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to comment #6)
> 
> Nick, could we just add an abort in monitorRecording if the number of slots in
> the current script is too high? Or did you have something else in mind?

I was thinking we'd abort as early as possible in the recording process.  That way the overhead would be low but we'd only need to code the check once.  This case should be very rare in practice anyway.

BTW, did you get the 256 value from anywhere in particular?  That's a lot less than 4,300...

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #7)
> I was thinking we'd abort as early as possible in the recording process.  That
> way the overhead would be low but we'd only need to code the check once.  This
> case should be very rare in practice anyway.

I guess we could stick a check in MonitorLoopEdge. Should we worry about the case where the loop calls a function with lots of slots?

> BTW, did you get the 256 value from anywhere in particular?  That's a lot less
> than 4,300...

Making up arbitrary constants is my favorite part of working at Mozilla. It's especially thrilling when I don't #define them ahead of time.

I guess to do this right, we should somehow measure the cost of snapshotting and weigh it against the gain from tracing. But then we'd have to estimate the number of snapshots that the trace would require. An alternate idea is to count the total number of slots snapshotted during the trace and abort if it hits some threshold. But then we waste more time before aborting.

Maybe we should instrument a browser to print the max number of slots seen while browsing, and just use double this number as our threshold.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #8)
> 
> I guess we could stick a check in MonitorLoopEdge. Should we worry about the
> case where the loop calls a function with lots of slots?

If it's difficult, and not worrying about it fixes this bug, then I wouldn't worry about it.

> Making up arbitrary constants is my favorite part of working at Mozilla. It's
> especially thrilling when I don't #define them ahead of time.

Good thing you're working on the profiler, then! :)

> I guess to do this right, we should somehow measure the cost of snapshotting
> and weigh it against the gain from tracing. But then we'd have to estimate the
> number of snapshots that the trace would require. An alternate idea is to count
> the total number of slots snapshotted during the trace and abort if it hits
> some threshold. But then we waste more time before aborting.
> 
> Maybe we should instrument a browser to print the max number of slots seen
> while browsing, and just use double this number as our threshold.

The latter sounds good -- ie. something not completely arbitrary, but let's not spend too much time on a rare case.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 489581 [details] [diff] [review]
profiler change

This really needs a different reviewer, like I said.  Picking one via 6-sided coin flip.

Comment 11

[David Mandelin [:dmandelin]]

Comment on attachment 489581 [details] [diff] [review]
profiler change

I think maxStackSlots isn't initialized. Looks like LoopProfile::reset is the place.

Otherwise, looks fine, although it would be nice if there was an additional pointer to the reader that |maxStackSlots| is a maximum observed value, as opposed to a maximum allowed value (which is my usual assumption when seeing 'max'). Adding 'seen' or 'observed' to the var name or comment would do it, I think.

Comment 12

[Nicholas Nethercote [inactive]]

TM's days are numbered:  WONTFIX.