Closed
Bug 1456672
Opened 6 years ago
Closed 6 years ago
Add comment to explain we do prefer to pass TimeStamp as a value instead of a reference
Categories
(Core :: Graphics, enhancement)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
Details
Attachments
(1 file, 5 obsolete files)
(I don't think 'Graphics' is the right component though, since some of them stand between layout and gfx.) TimeStamp is very thin class so it might not be a big deal, but I am not comfortable with that every time I see it. :/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f081ea1ebf46cba934bffe65662de55176ed64d
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8970772 [details] Bug 1456672 - Add a brief note that we prefer to pass TimeStamp objects by value. DONTBUILD https://reviewboard.mozilla.org/r/239526/#review245218
Attachment #8970772 -
Flags: review?(bugmail) → review+
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8970774 [details] Bug 1456672 - Use const TimeStamp& for arguments as possible in gfx/. https://reviewboard.mozilla.org/r/239530/#review245220
Attachment #8970774 -
Flags: review?(bugmail) → review+
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8970773 [details] Bug 1456672 - Use const TimeStamp& for RecordFontLoadDone(). https://reviewboard.mozilla.org/r/239528/#review245248
Attachment #8970773 -
Flags: review?(jfkthame) → review+
Comment 10•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > TimeStamp is very thin class so it might not be a big deal, but I am not > comfortable with that every time I see it. :/ Are we passing TimeStamp values down to lots of functions? I agree it's not a big deal, but it's going to generate (marginally) worse code on 64 bit builds (due to loading the TimeStampValue through a pointer). On 32 bit builds I guess it depends how deeply we pass these values through function calls, to see whether we save enough by avoiding pushing two words on the stack for each TimeStampValue. https://godbolt.org/g/aUHWQP
Assignee | ||
Comment 11•6 years ago
|
||
That's interesting... Anyway, the answer is yes, as far as I can tell. Most of the TimeStamp come from vsync timer, it's here [1], it's already const&. I haven't checked all the places so there might be some places in a few depth stacks. [1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/layout/ipc/VsyncChild.cpp#68
Assignee | ||
Comment 12•6 years ago
|
||
As per comment 10, I am not going to pass 'const TimeStamp&' here. And as as result of a brief chat with Cameron on IRC, I am going to re-use this bug to add a comment to TimeStamp class that explains we explicitly prefer to pass TimeStamp class as a value instead of reference.
Summary: Use const TimeStamp& for argument → Add comment to explain we do prefer to pass TimeStamp as a value instead of a reference
Assignee | ||
Comment 13•6 years ago
|
||
oh there are lots of place we pass TimeStamp as a reference. I am pretty sure I did do it in many places. :)
Comment 14•6 years ago
|
||
It's probably fine to say something like "Since TimeStamp objects are small, prefer to pass them by value unless there is a specific reason not to."? And I guess I wouldn't bother changing the existing callers unless you wanted to go to the effort, since it's not a huge deal either way.
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #14) > It's probably fine to say something like "Since TimeStamp objects are small, > prefer to pass them by value unless there is a specific reason not to."? Thanks! I am going to use the sentence as it is. (To be honest, I don't quite understand the difference between 'by value' and 'as value'.) :-) > And I guess I wouldn't bother changing the existing callers unless you > wanted to go to the effort, since it's not a huge deal either way. Yeah, I am not going to do that, I hope someone tries to make them consistent.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8970773 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8970774 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8970775 -
Attachment is obsolete: true
Attachment #8970775 -
Flags: review?(cam)
Assignee | ||
Updated•6 years ago
|
Attachment #8970776 -
Attachment is obsolete: true
Attachment #8970776 -
Flags: review?(cam)
Assignee | ||
Updated•6 years ago
|
Attachment #8970777 -
Attachment is obsolete: true
Attachment #8970777 -
Flags: review?(cam)
Comment 17•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15) > Thanks! I am going to use the sentence as it is. (To be honest, I don't > quite understand the difference between 'by value' and 'as value'.) :-) Both are understandble, but I think "pass by value", "pass by reference", etc. are more common expressions.
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8970772 [details] Bug 1456672 - Add a brief note that we prefer to pass TimeStamp objects by value. DONTBUILD https://reviewboard.mozilla.org/r/239526/#review245582 ::: mozglue/misc/TimeStamp.h:408 (Diff revision 2) > * is initialized to the clock's epoch and provides a > * time_since_epoch() method that functions similiarly. i.e. > * t.IsNull() is equivalent to t.time_since_epoch() == decltype(t)::duration::zero(); > + * > + * Note that, since TimeStamp objects are small, prefer to pass them by value > + * unless there is a specific reason not to so. "specific reason not to" or "specific reason not to do so"
Attachment #8970772 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/20ffaafbae3c Add a brief note that we prefer to pass TimeStamp objects by value. r=heycam,kats DONTBUILD
Comment 21•6 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #10) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > > TimeStamp is very thin class... Note that on Windows it's not quite as thin as on other platforms: https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/mozglue/misc/TimeStamp_windows.h#22-25 With this class, passing by value will involve pushing five (32-bit) words onto the stack, rather than just two. Is that enough to tilt the balance towards passing by const ref instead?
Comment 22•6 years ago
|
||
Ouch, OK, that makes me change my mind, yes...
Assignee | ||
Comment 23•6 years ago
|
||
Oh, that's defined in a different file. I didn't notice that.
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/20ffaafbae3c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Assignee: nobody → hikezoe
You need to log in
before you can comment on or make changes to this bug.
Description
•