Closed Bug 1245954 Opened 4 years ago Closed 4 years ago

Console StartTimer/StopTimer and IncrementCounter should run in the owning thread

Categories

(Core :: DOM: Core & HTML, defect)

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch console1.patch (obsolete) — Splinter Review
No description provided.
Attachment #8715967 - Flags: review?(bzbarsky)
Attached patch console1.patch (obsolete) — Splinter Review
Attachment #8715967 - Attachment is obsolete: true
Attachment #8715967 - Flags: review?(bzbarsky)
Attachment #8716202 - Flags: review?(bzbarsky)
Blocks: 1246091
Comment on attachment 8716202 [details] [diff] [review]
console1.patch

Please document the new members and then request review again.  It will save a lot of time in the long run...
Attachment #8716202 - Flags: review?(bzbarsky)
Attached patch console1.patch (obsolete) — Splinter Review
Attachment #8716202 - Attachment is obsolete: true
Attachment #8716738 - Flags: review?(bzbarsky)
Attached patch console1.patch (obsolete) — Splinter Review
Attachment #8716738 - Attachment is obsolete: true
Attachment #8716738 - Flags: review?(bzbarsky)
Attachment #8716739 - Flags: review?(bzbarsky)
Comment on attachment 8716739 [details] [diff] [review]
console1.patch

This is still missing the basic documentation for the new data members that I asked for in comment 2...
Attachment #8716739 - Flags: review?(bzbarsky)
Attached patch console1.patch (obsolete) — Splinter Review
Attachment #8716739 - Attachment is obsolete: true
Attachment #8717344 - Flags: review?(bzbarsky)
Attached patch console1.patchSplinter Review
Attachment #8717344 - Attachment is obsolete: true
Attachment #8717344 - Flags: review?(bzbarsky)
Attachment #8717506 - Flags: review?(bzbarsky)
Comment on attachment 8717506 [details] [diff] [review]
console1.patch

>+++ b/dom/base/Console.cpp
>+  // These values are set in the owning thread and they contain the timestamp,

Against what timebase?  Is there a reason this is a DOMHighResTimeStamp, not a mozilla::TimeStamp?  Please document.

> and the status of the creation of a new timer by StartTimer.

What does the status boolean actually mean?

I assume these members are only used for some values of mMethodName?  Please document for which ones.

>+  // These values are set in the owning thread and they contain the duration,
>+  // the name and the status of the StopTimer method.

Again, what does the status boolean actually mean?  And again, which mMethodName values are these relevant to?

>+  // These 2 values are set by IncreaseCounter on the owning thread and they are
>+  // used on the main-thread by CreateCounterValue.

Again, which mMethodName values are these relevant to?

For all of these, since they are completely unsynchronized, please document why multithreaded access to them is actually safe (e.g. that they're set on the owning thread, then we hand off the ConsoleCallData to the main thread, and never touch them on the owning thread again or something).

>+++ b/dom/base/Console.h
>+  // aTimerValue. It returns false if there is a JS exception is thrown or if

s/if there is a JS exception/if a JS exception/

>+  //                window->performance.now() or from Now() -
>+  //                workerPrivate->CreationTimeStamp() in workers.

Shouldn't that use NowBaseTimeStamp() instead of CreationTimeStamp()?

>+  // * aTimerLabel - This label will be populated with the aName as string.

s/the aName as string/aName converted to a string/

>+  // CreateStartTimerValue is called on the main-thread only and generates a

s/main-thread/main thread/ and similar elsewhere.

>It's called only after
>+  // the execution StartTimer on the owning thread.

Ah, here's some of the documentation about member access...  Probably worth saying that this is called only after StartTime has finished executing.

>+  // * aTimerLabel - this label must be what StartTimer receives as aTimerLabel.

s/receives/received/

>+  // * aTimerValue - this is what StartTimer receives as aTimerValue

s/receives/received/

>+  // This StopTimer follows the same pattern of StartTimer: it runs on the

s/This StopTimer/StopTimer/
s/pattern of/pattern as/

>+  // exception is thrown or if the aName timer doesn't exist in the
>+  // mTimerRegistry.

s/the mTimerRegistry/mTimerRegistry/

>+  // * aTimerLabel - This label will be populated with the aName as string.

s/the aName as string/aName converted to a string/

>+  //                    started (read StartTimer).

s/read/see/

>+  // false. Read StopTimer.

s/Read/See/

>+  // * aTimerLabel - this label must be what StopTimer receives as
> aTimerLabel.

s/receives/received/

>+  // * aTimerDuration - this is what StopTimer receives as aTimerDuration

s/receives/received/

>+  // This method follows the same pattern of StartTimer: its runs on the

s/pattern of/pattern as/

>+  // thread and populate aCountLabel, used by CreateCounterValue on the

s/populate/populates/

>+  // main-thread. Returns MAX_PAGE_COUNTERS in case of error.

What does it return in case of not error?

>+  // ConsoleCounterError instead. Read IncreaseCounter.

s/Read/See/

>+  // * aCountLabel - this label must be what IncreaseCounter receives as

s/receives/received/

>+  // Touched on the owned thread.

s/owned owner/

>   nsDataHashtable<nsStringHashKey, uint32_t> mCounterRegistry;

Where is this touched now?

r=me with the above comment fixes plus sorting out the worker thing regarding start time vs now base time.
Attachment #8717506 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/5523afd171d8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.