1224123 - Tracelogger: fix the use of LastEntryId in tracelogger.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

In TraceLogger.h we still used the lastEntryId, which could give incorrect info when we just flushed (e.g. size == 0).
-> This is used by some hooks in debugger. Fix it by saving size() instead.
https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#4465
https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#4370


bonus: test for the slow script dialog in the debugger loop:
https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#4483

Comment 1

[Wei Wu [:wuwei UTC+8]]

(In reply to Hannes Verschore [:h4writer] from comment #0)
> In TraceLogger.h we still used the lastEntryId, which could give incorrect
> info when we just flushed (e.g. size == 0).
> -> This is used by some hooks in debugger. Fix it by saving size() instead.
> https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#4465
> https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#4370
> 
> 
> bonus: test for the slow script dialog in the debugger loop:
> https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#4483

I couldn't find the use of lastEntryId in js/src/vm/Debugger* ... maybe you mean vm/TraceLogging* ?

Comment 2

[Hannes Verschore [:h4writer]]

In js/src/vm/Debugger.cpp/.h

both 'traceLoggerScriptedCallsLastDrainedId' and 'traceLoggerLastDrainedId' are actually lastEntryId.

Comment 3

[Wei Wu [:wuwei UTC+8]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> In js/src/vm/Debugger.cpp/.h
> 
> both 'traceLoggerScriptedCallsLastDrainedId' and 'traceLoggerLastDrainedId'
> are actually lastEntryId.

I'm happy to steal this bug, while I'm afraid that the use of lastEntryId in Debugger.cpp have no problems. In tracelogger, events.size() are always large than 1. The trick is that every time events get flushed (clear()), one event will be pushed in into it. (Or am I still seeing the wrong place?)

Comment 4

[Wei Wu [:wuwei UTC+8]]

Attachment: some trivial cleanup

not bug-fix patch. just a few cleanup.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8688882 [details] [diff] [review]
some trivial cleanup

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

If a patch is ready for commit, feel free to request r? instead of f?.
More removal of lastEntryId \0/

Comment 6

[Wei Wu [:wuwei UTC+8]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> Comment on attachment 8688882 [details] [diff] [review]
> some trivial cleanup
> 
> Review of attachment 8688882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If a patch is ready for commit, feel free to request r? instead of f?.
> More removal of lastEntryId \0/

ok :)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6302c85d9ce9

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6302c85d9ce9

Comment 9

[Hannes Verschore [:h4writer]]

Not fixed yet. That's why I still have this ni request ;)

Comment 10

[Wei Wu [:wuwei UTC+8]]

Ah, sorry for inconvenience. I should set a 'leave-open' keyword on this bug.

(In reply to Hannes Verschore [:h4writer] from comment #9)
> Not fixed yet. That's why I still have this ni request ;)

So I got it wrong. I was thinking that the remaining use of lastEntryId were reasonable,
as described in comment 3. The wrong assumption I had made is that ContinuousSpace<T>::clear()
could only be called by TraceLoggingThread::log(). Turned out the Debugger
has the ability to flush events anytime it want.

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Change "lastId" to "size"

This removes the use for "lastId" and uses "size".
This fixes a bug, since "lastId" cannot be called when there are no items in 'events' yet. As a result we moved away of using "lastId" to use "events.size()".
Which is lastId + 1, but with no undefined behaviour when there are no items.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8697036 [details] [diff] [review]
Change "lastId" to "size"

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

::: js/src/vm/TraceLogging.h
@@ +237,3 @@
>          // If still logging in the same iteration, there are no lost events.
>          if (lastIteration == iteration_) {
> +            MOZ_ASSERT(lastSize < events.size());

Shouldn't this be <= here as well?

@@ +244,2 @@
>          // the maximum capacity there are no logs that are lost.
> +        if (lastIteration + 1 == iteration_ && lastSize == events.capacity())

There's a comment above the definition of iteration_ which wants an update.

I was trying to remember this code. Can we invert the first part of the condition (it seems to make thinking more natural to have lastIteration == iteration_ - 1), please?

Also, why are we checking capacity()? Is it because we assume the capacity is an upperbound of the former length before flushing? It sounds inaccurate... Maybe we should just be using a second counter here as well? Or can you explain in details (maybe in a comment) why this will always work?

Comment 13

[Wei Wu [:wuwei UTC+8]]

Hi h4writer I'm kinda busy these days (struggling with my thesis :( ) and will come back before May. Feel free to take my bugs if necessary :)
btw should we close this bug now? seems like it had been fixed.

Comment 14

[Hannes Verschore [:h4writer]]

Should this last patch still get landed?

Comment 15

[Hannes Verschore [:h4writer]]

Seems like this last patch landed as bug 1231170.