Closed Bug 1324434 Opened 7 years ago Closed 7 years ago

remove JS embedder time code

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(1 file)

Bug 1324434 - remove JS_GetCurrentEmbedderTime in favor of TimeStamp;
59 bytes, text/x-review-board-request
sfink
: review+
Details 
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 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+
(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 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.
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)
(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 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 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+
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.
(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.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0afe5ed6c1b4
remove JS_GetCurrentEmbedderTime in favor of TimeStamp; r=sfink
https://hg.mozilla.org/mozilla-central/rev/0afe5ed6c1b4
Status: ASSIGNED → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: