Closed Bug 1224123 Opened 7 years ago Closed 6 years ago

Tracelogger: fix the use of LastEntryId in tracelogger.h

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files)

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
Flags: needinfo?(hv1989)
(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* ?
In js/src/vm/Debugger.cpp/.h

both 'traceLoggerScriptedCallsLastDrainedId' and 'traceLoggerLastDrainedId' are actually lastEntryId.
(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?)
not bug-fix patch. just a few cleanup.
Attachment #8688882 - Flags: feedback?(hv1989)
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/
Attachment #8688882 - Flags: feedback?(hv1989) → review+
(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 :)
Assignee: nobody → lazyparser
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6302c85d9ce9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Not fixed yet. That's why I still have this ni request ;)
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
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.
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.
Flags: needinfo?(hv1989)
Attachment #8697036 - Flags: review?(benj)
Blocks: 1231170
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?
Attachment #8697036 - Flags: review?(benj) → review+
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.
Assignee: lazyparser → hv1989
Should this last patch still get landed?
Flags: needinfo?(hv1989)
Seems like this last patch landed as bug 1231170.
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Flags: needinfo?(hv1989)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.