Closed
Bug 1324434
Opened 8 years ago
Closed 8 years ago
remove JS embedder time code
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: tromey, Assigned: tromey)
References
Details
Attachments
(1 file)
JS_CurrentEmbedderTimeFunction, JS_SetCurrentEmbedderTimeFunction, and
JS_GetCurrentEmbedderTime were added because, at the time, TimeStamp
was not directly available in js/. However, now that TimeStamp has been
moved to mozglue, this code can be removed.
I'm making this depend on bug 1173354 to avoid clashing with changes going
on there.
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8823632 [details]
Bug 1324434 - remove JS_GetCurrentEmbedderTime in favor of TimeStamp;
https://reviewboard.mozilla.org/r/102198/#review102632
Awesome, thanks! Note that this will have to be rebased over what finally landed, since I made at least one of these changes too (or rather, I removed startTimestamp/endTimestamp and made start/end be TimeStamps.) And I'm still living in fear that it'll get backed out due to the intermittent failures I never managed to track down on XP. (I landed some better diagnostics for that.) Which would be from a different change, but it can't really be backed out separately. Hopefully the switch to TimeStamp fixed that problem. (Or maybe we're not testing XP anymore? That would fix it too!)
Attachment #8823632 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> Awesome, thanks! Note that this will have to be rebased over what finally
> landed
For some mysterious reason my branch was based on the defunct fx-team rather than
m-c, which would have avoided this.
Comment hidden (mozreview-request) |
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8823632 [details]
Bug 1324434 - remove JS_GetCurrentEmbedderTime in favor of TimeStamp;
https://reviewboard.mozilla.org/r/102198/#review103046
The mozreview interdiff didn't work (it showed the rebase goop), but the patch is pretty small now and it still looks good to me.
Assignee | ||
Comment 6•8 years ago
|
||
It fails Memory-drainAllocationsLog-14.js because that test compares allocation log
times against dateNow, which works currently because when the embedder doesn't set the
time function, the two are equivalent.
One option would be to change the shell's dateNow to use time-since-process-creation.
However I wasn't sure if this is really ok. Is it?
The other option would be to expose the time-since-creation in TestingFunctions.cpp.
Which do you think is preferable?
Flags: needinfo?(sphink)
Comment 7•8 years ago
|
||
(In reply to Tom Tromey :tromey from comment #6)
> One option would be to change the shell's dateNow to use
> time-since-process-creation.
> However I wasn't sure if this is really ok. Is it?
>
> The other option would be to expose the time-since-creation in
> TestingFunctions.cpp.
>
> Which do you think is preferable?
On the face of it, it seems like making dateNow diverge wildly from Date.now would be pretty surprising. I'm going to wimp out and opt for option #2.
Flags: needinfo?(sphink)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8823632 [details]
Bug 1324434 - remove JS_GetCurrentEmbedderTime in favor of TimeStamp;
I think this warrants re-review given the addition.
Attachment #8823632 -
Flags: review+ → review?(sphink)
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8823632 [details]
Bug 1324434 - remove JS_GetCurrentEmbedderTime in favor of TimeStamp;
https://reviewboard.mozilla.org/r/102198/#review108790
::: js/src/builtin/TestingFunctions.cpp:4619
(Diff revision 3)
> "disableForEach()",
> " Disables the deprecated, non-standard for-each.\n"),
>
> + JS_FN_HELP("timeSinceCreation", TimeSinceCreation, 0, 0,
> +"TimeSinceCreation()",
> +" Returns the time since process creation, in units compatible with the profiler.\n"),
The units are milliseconds, which should be mentioned. "using a clock compatible with the profiler" maybe?
Attachment #8823632 -
Flags: review?(sphink) → review+
Comment 11•8 years ago
|
||
It's failing on try, which means I'm probably not doing a good job of reviewing here, but I didn't spot the problem offhand.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #11)
> It's failing on try, which means I'm probably not doing a good job of
> reviewing here, but I didn't spot the problem offhand.
That was from the old patch.
I started a try run for the current patch just now.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
Another rebase, this time with a cleaner try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0597adbbe5839b2a97c124e95043f3b6c3618aae
Comment 18•8 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0afe5ed6c1b4
remove JS_GetCurrentEmbedderTime in favor of TimeStamp; r=sfink
Comment 19•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•