Closed Bug 1465505 Opened Last year Closed Last year

GC still uses PRMJ_Now() rather than mozilla::TimeStamp

Categories

(Core :: JavaScript: GC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jonco, Assigned: wcosta, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files, 3 obsolete files)

Use of PRMJ_Now() in the GC should be replaced by mozilla::TimeStamp.
Summary: GC still used PRMJ_Now() rather than mozilla::TimeStamp → GC still uses PRMJ_Now() rather than mozilla::TimeStamp
Looks like there are 6 occurrences within GC.cpp
I'll take this.  Where are the instructions for submitting a patch?  I'm assuming I'll need to create a new branch and name it with this bug's ID number and then submit a pull request.
(In reply to Jonah Ho (AAstar) from comment #2)
Great!  Here's a high-level overview:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

I can review.  Let me know if you have any more specific questions.
Assignee: nobody → ho.jonah
Ops, I wrote a patch but didn't see someone else took it...
Comment on attachment 8985786 [details]
Bug 1465505: Replace PRMJ_Now() by mozilla::TimeStamp

https://reviewboard.mozilla.org/r/251314/#review257628

::: js/src/vm/Realm.h:424
(Diff revision 1)
>       * from the self-hosting global.
>       */
>      js::ReadBarrieredScriptSourceObject selfHostingScriptSource { nullptr };
>  
>      // Last time at which an animation was played for this realm.
> -    int64_t lastAnimationTime = 0;
> +    mozilla::TimeStamp lastAnimationTime;

Can you make this MainThreadData<> too?  Also in JSRuntime below.
(In reply to Wander Lairson Costa [:wcosta] from comment #5)
> Ops, I wrote a patch but didn't see someone else took it...

No problem, please assign it to yourself.  I'll take another one.
Assignee: ho.jonah → wcosta
Status: NEW → ASSIGNED
Attachment #8985786 - Attachment is obsolete: true
Attachment #8985786 - Flags: review?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #6)
> Comment on attachment 8985786 [details]
> Bug 1465505: Replace PRMJ_Now() by mozilla::TimeStamp
> 
> https://reviewboard.mozilla.org/r/251314/#review257628
> 
> ::: js/src/vm/Realm.h:424
> (Diff revision 1)
> >       * from the self-hosting global.
> >       */
> >      js::ReadBarrieredScriptSourceObject selfHostingScriptSource { nullptr };
> >  
> >      // Last time at which an animation was played for this realm.
> > -    int64_t lastAnimationTime = 0;
> > +    mozilla::TimeStamp lastAnimationTime;
> 
> Can you make this MainThreadData<> too?  Also in JSRuntime below.

Done :)
Comment on attachment 8985840 [details]
Bug 1465505: Replace PRMJ_Now() by mozilla::TimeStamp

https://reviewboard.mozilla.org/r/251320/#review257670

Great, thanks for taking this.
Attachment #8985840 - Flags: review?(jcoppeard) → review+
Pushed by wcosta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c115f0bb2bfb
Replace PRMJ_Now() by mozilla::TimeStamp r=jonco
Backed out changeset c115f0bb2bfb (bug 1465505) spidermonkey bustages.

Backout: https://hg.mozilla.org/integration/autoland/rev/ef06853a18a59002945d9ebb29944296f9da2dcc

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c115f0bb2bfbfc808f7a09a03f808aa80aeabfde&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=183739415

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183739415&repo=autoland

[task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py | /builds/worker/workspace/build/src/dom/bindings/Errors.msg | ok
[task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py | /builds/worker/workspace/build/src/js/src/ctypes/ctypes.msg | ok
[task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py | /builds/worker/workspace/build/src/js/src/js.msg | ok
[task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py | /builds/worker/workspace/build/src/js/src/jsshell.msg | ok
[task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py | /builds/worker/workspace/build/src/js/xpconnect/src/jsshell.msg | ok
[task 2018-06-19T03:39:12.639Z] make[2]: Leaving directory '/builds/worker/workspace/build/src/obj-spider/js/src'
[task 2018-06-19T03:39:12.644Z] make[2]: Entering directory '/builds/worker/workspace/build/src/obj-spider/js/src/build'
[task 2018-06-19T03:39:12.644Z] /builds/worker/workspace/build/src/obj-spider/_virtualenvs/init/bin/python /builds/worker/workspace/build/src/config/check_vanilla_allocations.py libjs_static.a
[task 2018-06-19T03:39:13.314Z] TEST-PASS | check_vanilla_allocations.py | ok
[task 2018-06-19T03:39:13.321Z] make[2]: Leaving directory '/builds/worker/workspace/build/src/obj-spider/js/src/build'
[task 2018-06-19T03:39:13.321Z] make[1]: Leaving directory '/builds/worker/workspace/build/src/obj-spider'
[task 2018-06-19T03:39:13.322Z] in directory /builds/worker/workspace/build/src/obj-spider, running ['setarch', 'x86_64', '-R', 'make', 'check-jit-test']
[task 2018-06-19T03:39:13.330Z] make -C js/src check-jit-test
[task 2018-06-19T03:39:13.439Z] make[1]: Entering directory '/builds/worker/workspace/build/src/obj-spider/js/src'
[task 2018-06-19T03:39:13.439Z] ../../dist/bin/run-mozilla.sh /builds/worker/workspace/build/src/obj-spider/_virtualenvs/init/bin/python -u /builds/worker/workspace/build/src/js/src/jit-test/jit_test.py \
[task 2018-06-19T03:39:13.439Z]         --no-slow --no-progress --format=automation --jitflags=all \
[task 2018-06-19T03:39:13.439Z] 		 \
[task 2018-06-19T03:39:13.439Z] 		 \
[task 2018-06-19T03:39:13.439Z]         ../../dist/bin/js 
[task 2018-06-19T03:39:16.656Z] {"action": "suite_start", "pid": 4704, "source": "jittests", "tests": [], "thread": "main", "time": 1529379556.655957}
[task 2018-06-19T03:39:16.687Z] Assertion failure: !IsNull() (Cannot compute with a null value), at /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:528
[task 2018-06-19T03:39:16.687Z] Exit code: -11
[task 2018-06-19T03:39:16.687Z] FAIL - backup-point-bug1315634.js
[task 2018-06-19T03:39:16.687Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/backup-point-bug1315634.js | Assertion failure: !IsNull() (Cannot compute with a null value), at /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:528 (code -11, args "") [0.0 s]
[task 2018-06-19T03:39:16.687Z] {"action": "test_start", "jitflags": "", "pid": 4710, "source": "jittests", "test": "backup-point-bug1315634.js", "thread": "main", "time": 1529379556.668268}
[task 2018-06-19T03:39:16.687Z] {"action": "test_end", "extra": {"jitflags": "", "pid": 4710}, "jitflags": "", "message": "Assertion failure: !IsNull() (Cannot compute with a null value), at /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:528", "pid": 4710, "source": "jittests", "status": "FAIL", "test": "backup-point-bug1315634.js", "thread": "main", "time": 1529379556.687594}
[task 2018-06-19T03:39:16.687Z] INFO exit-status     : -11
[task 2018-06-19T03:39:16.687Z] INFO timed-out       : False
[task 2018-06-19T03:39:16.687Z] INFO stderr         2> Assertion failure: !IsNull() (Cannot compute with a null value), at /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:528
[task 2018-06-19T03:39:16.687Z] Assertion failure: !IsNull() (Cannot compute with a null value), at /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.h:528
[task 2018-06-19T03:39:16.687Z] Exit code: -11
[task 2018-06-19T03:39:16.687Z] FAIL - backup-point-bug1315634.js
Flags: needinfo?(wcosta)
(In reply to Cristian Brindusan [:cbrindusan] from comment #12)
> Backed out changeset c115f0bb2bfb (bug 1465505) spidermonkey bustages.
> 
> Backout:
> https://hg.mozilla.org/integration/autoland/rev/
> ef06853a18a59002945d9ebb29944296f9da2dcc
> 
> Push with failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=c115f0bb2bfbfc808f7a09a03f808aa80aeabfde&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=success&selectedJob=183739415
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=183739415&repo=autoland
> 
> [task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py |
> /builds/worker/workspace/build/src/dom/bindings/Errors.msg | ok
> [task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py |
> /builds/worker/workspace/build/src/js/src/ctypes/ctypes.msg | ok
> [task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py |
> /builds/worker/workspace/build/src/js/src/js.msg | ok
> [task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py |
> /builds/worker/workspace/build/src/js/src/jsshell.msg | ok
> [task 2018-06-19T03:39:12.632Z] TEST-PASS | check_js_msg_encoding.py |
> /builds/worker/workspace/build/src/js/xpconnect/src/jsshell.msg | ok
> [task 2018-06-19T03:39:12.639Z] make[2]: Leaving directory
> '/builds/worker/workspace/build/src/obj-spider/js/src'
> [task 2018-06-19T03:39:12.644Z] make[2]: Entering directory
> '/builds/worker/workspace/build/src/obj-spider/js/src/build'
> [task 2018-06-19T03:39:12.644Z]
> /builds/worker/workspace/build/src/obj-spider/_virtualenvs/init/bin/python
> /builds/worker/workspace/build/src/config/check_vanilla_allocations.py
> libjs_static.a
> [task 2018-06-19T03:39:13.314Z] TEST-PASS | check_vanilla_allocations.py | ok
> [task 2018-06-19T03:39:13.321Z] make[2]: Leaving directory
> '/builds/worker/workspace/build/src/obj-spider/js/src/build'
> [task 2018-06-19T03:39:13.321Z] make[1]: Leaving directory
> '/builds/worker/workspace/build/src/obj-spider'
> [task 2018-06-19T03:39:13.322Z] in directory
> /builds/worker/workspace/build/src/obj-spider, running ['setarch', 'x86_64',
> '-R', 'make', 'check-jit-test']
> [task 2018-06-19T03:39:13.330Z] make -C js/src check-jit-test
> [task 2018-06-19T03:39:13.439Z] make[1]: Entering directory
> '/builds/worker/workspace/build/src/obj-spider/js/src'
> [task 2018-06-19T03:39:13.439Z] ../../dist/bin/run-mozilla.sh
> /builds/worker/workspace/build/src/obj-spider/_virtualenvs/init/bin/python
> -u /builds/worker/workspace/build/src/js/src/jit-test/jit_test.py \
> [task 2018-06-19T03:39:13.439Z]         --no-slow --no-progress
> --format=automation --jitflags=all \
> [task 2018-06-19T03:39:13.439Z] 		 \
> [task 2018-06-19T03:39:13.439Z] 		 \
> [task 2018-06-19T03:39:13.439Z]         ../../dist/bin/js 
> [task 2018-06-19T03:39:16.656Z] {"action": "suite_start", "pid": 4704,
> "source": "jittests", "tests": [], "thread": "main", "time":
> 1529379556.655957}
> [task 2018-06-19T03:39:16.687Z] Assertion failure: !IsNull() (Cannot compute
> with a null value), at
> /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.
> h:528
> [task 2018-06-19T03:39:16.687Z] Exit code: -11
> [task 2018-06-19T03:39:16.687Z] FAIL - backup-point-bug1315634.js
> [task 2018-06-19T03:39:16.687Z] TEST-UNEXPECTED-FAIL |
> js/src/jit-test/tests/backup-point-bug1315634.js | Assertion failure:
> !IsNull() (Cannot compute with a null value), at
> /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.
> h:528 (code -11, args "") [0.0 s]
> [task 2018-06-19T03:39:16.687Z] {"action": "test_start", "jitflags": "",
> "pid": 4710, "source": "jittests", "test": "backup-point-bug1315634.js",
> "thread": "main", "time": 1529379556.668268}
> [task 2018-06-19T03:39:16.687Z] {"action": "test_end", "extra": {"jitflags":
> "", "pid": 4710}, "jitflags": "", "message": "Assertion failure: !IsNull()
> (Cannot compute with a null value), at
> /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.
> h:528", "pid": 4710, "source": "jittests", "status": "FAIL", "test":
> "backup-point-bug1315634.js", "thread": "main", "time": 1529379556.687594}
> [task 2018-06-19T03:39:16.687Z] INFO exit-status     : -11
> [task 2018-06-19T03:39:16.687Z] INFO timed-out       : False
> [task 2018-06-19T03:39:16.687Z] INFO stderr         2> Assertion failure:
> !IsNull() (Cannot compute with a null value), at
> /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.
> h:528
> [task 2018-06-19T03:39:16.687Z] Assertion failure: !IsNull() (Cannot compute
> with a null value), at
> /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/TimeStamp.
> h:528
> [task 2018-06-19T03:39:16.687Z] Exit code: -11
> [task 2018-06-19T03:39:16.687Z] FAIL - backup-point-bug1315634.js

I am in PTO now, will look at it when I am back.
Flags: needinfo?(wcosta)
Attachment #8985840 - Attachment is obsolete: true
The problem with the old patch is that I tested it only on release builds, in debug it triggered some assertions. I ran all jit and js tests and fixed all assertions that came up.
Comment on attachment 8989463 [details]
Bug 1465505: Replace PRMJ_Now() by mozilla::TimeStamp

https://reviewboard.mozilla.org/r/254514/#review261546

Thanks for the updates.  LGTM, just a couple of nits.

::: commit-message-a0e47:5
(Diff revision 1)
> +Bug 1465505: Replace PRMJ_Now() by mozilla::TimeStamp r=jonco
> +
> +Notice as TimeStamp is not an integral type, it can't be wrapped by
> +mozilla::Atomic. There two uses of it, which seems to not harm the code,
> +but I am not entirily sure.

We've made these MainThreadData<> now so this adds an assertion that this data is only accessed on the main thread.

::: js/src/gc/GC.cpp:308
(Diff revision 1)
>  
>      /* JSGC_DYNAMIC_HEAP_GROWTH */
>      static const bool DynamicHeapGrowthEnabled = false;
>  
>      /* JSGC_HIGH_FREQUENCY_TIME_LIMIT */
> -    static const uint64_t HighFrequencyThresholdUsec = 1000000;
> +    static const auto HighFrequencyThresholdUsec = mozilla::TimeDuration::FromMicroseconds(1000000);

nit: We can lose the ...Usec part of the name now that it's a TimeDuration.  Also, can you make this FromSeconds(1) as that's clearer?

::: js/src/gc/Scheduling.h:453
(Diff revision 1)
>      size_t gcZoneAllocThresholdBase() const { return gcZoneAllocThresholdBase_; }
>      double allocThresholdFactor() const { return allocThresholdFactor_; }
>      double allocThresholdFactorAvoidInterrupt() const { return allocThresholdFactorAvoidInterrupt_; }
>      size_t zoneAllocDelayBytes() const { return zoneAllocDelayBytes_; }
>      bool isDynamicHeapGrowthEnabled() const { return dynamicHeapGrowthEnabled_; }
> -    uint64_t highFrequencyThresholdUsec() const { return highFrequencyThresholdUsec_; }
> +    const mozilla::TimeDuration &highFrequencyThresholdUsec() const { return highFrequencyThresholdUsec_; }

Please remove ...Usec here too.
Attachment #8989463 - Flags: review?(jcoppeard)
Also I'm not clear on whether it's best practice to pass TimeStamps by value or by const reference.  We seem to do both in gecko, and the patch is fine as it is.
Attachment #8989463 - Attachment is obsolete: true
(In reply to Jon Coppeard (:jonco) from comment #17)
> Also I'm not clear on whether it's best practice to pass TimeStamps by value
> or by const reference.  We seem to do both in gecko, and the patch is fine
> as it is.

(At least on Linux) TimeStamp has the size of an unint64_t, therefore, in this case, it won't make much difference I guess, and since people use const reference by default for non POD types, I am sticking with it. We could always add some TMP tricks to let the compiler select the best, but I don't think the added complexity pays off :)
I see a lot of test failures on my try push, but it doesn't seem to be related to this patch.
(In reply to Wander Lairson Costa [:wcosta] from comment #19)
> (In reply to Jon Coppeard (:jonco) from comment #17)
> > Also I'm not clear on whether it's best practice to pass TimeStamps by value
> > or by const reference.  We seem to do both in gecko, and the patch is fine
> > as it is.
> 
> (At least on Linux) TimeStamp has the size of an unint64_t, therefore, in
> this case, it won't make much difference I guess, and since people use const
> reference by default for non POD types, I am sticking with it. We could
> always add some TMP tricks to let the compiler select the best, but I don't
> think the added complexity pays off :)

s/POD/integral/
Comment on attachment 8989842 [details]
Bug 1465505: Replace PRMJ_Now() by mozilla::TimeStamp

https://reviewboard.mozilla.org/r/254806/#review261802

Great, thank you for the patch.
Attachment #8989842 - Flags: review?(jcoppeard) → review+
Pushed by wcosta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05329bb2ebf8
Replace PRMJ_Now() by mozilla::TimeStamp r=jonco
https://hg.mozilla.org/mozilla-central/rev/05329bb2ebf8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.