Closed
Bug 1334307
Opened 7 years ago
Closed 7 years ago
GenericPrinter::vprintf does not need to allocate
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: tromey, Assigned: tromey)
References
Details
Attachments
(1 file)
After bug 1060419 lands, GenericPrinter::vprintf will not need to allocate and then free the formatted output. Instead it can subclass mozilla::PrintfTarget and provide an implementation that calls put() as characters are emitted. This would be more efficient; I don't know if this is important, but I discovered this while auditing printf-likes, and figured a bug would be harmless.
Assignee | ||
Comment 1•7 years ago
|
||
One weirdness here is that the return value of |put| is treated differently by the different subclasses of GenericPrinter. Sprinter::put returns the offset of the data just written, while Fprinter::put and LSprinter::put return the length of the data written. The latter two contradict the comment before GenericPrinter::put. The main upshot of this is that I was thinking of removing some of the overrides of printf and vprintf, but this doesn't seem possible given the inconsistency.
Comment 2•7 years ago
|
||
When I added the Fprinter::put and LSprinter::put, I followed the same idea as the putc function, which is to return the number of character written. I don't think computing or saving an offset for Fprinter and LSprinter makes any sense, as they are not trivially seekable. Based on the uses of these put function, it seems that only the (< 0) and (>= 0) aspect of it matters. I would be fine about changing the comment to say "returns -1 on errors, or any non-negative value if any bytes is written".
Assignee | ||
Comment 3•7 years ago
|
||
Thanks for that; I avoided looking at the callers, but now that I do I see some weirdness. For example: https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/js/src/jsopcode.cpp#2209 ... this will not actually detect an error from the call. Most callers are correct but you can find plenty that are not here: https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22js%3A%3AGenericPrinter%3A%3Aput%28const+char+%2A%29%22 I wonder what you'd think about a patch to change |put| and *printf to return bool instead? I could do that as a precursor to the non-allocating fix.
Flags: needinfo?(nicolas.b.pierron)
Comment 4•7 years ago
|
||
(In reply to Tom Tromey :tromey from comment #3) > I wonder what you'd think about a patch to change |put| and *printf to > return bool > instead? I could do that as a precursor to the non-allocating fix. Definitely yes!
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Comment 5•7 years ago
|
||
This is a bit cleaner to do once bug 1341880 is fixed.
Depends on: 1341880
Assignee | ||
Comment 6•7 years ago
|
||
I've also changed the code to remove some now-redundant overrides.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
The blocker is r+ and in try so I'll reattach this patch.
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8842525 [details] Bug 1334307 - do not allocate in GenericPrinter::vprintf; https://reviewboard.mozilla.org/r/116334/#review118278 This looks good, and the way to go. But … ::: commit-message-db9c7:1 (Diff revision 2) > +Bug 1334307 - do not allocate in GenericPrinter::vprintf; r?nbp remove this file. ::: js/src/vm/Printer.cpp:456 (Diff revision 2) > - > -bool > Fprinter::vprintf(const char* fmt, va_list ap) > { > MOZ_ASSERT(file_); > bool r = vfprintf(file_, fmt, ap); I think we should remove the Fprinter::vprintf command, such that we no longer depend on the system printf at all and that the format string is only handled by our code. On the same note, I think it would be good if the GenericPrinterPrintTarget could have a fixed size buffer, such that we can avoid frequent calls to LSprinter::put and Fprinter::put.
Attachment #8842525 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 12•7 years ago
|
||
From irc: <nbp> tromey: for Bug 1334307, I am not completely sure about the performance concern. it might be that this is not a reall concern either. [09:40] <firebot> https://bugzil.la/1334307 — NEW, ttromey@mozilla.com — GenericPrinter::vprintf does not need to allocate <nbp> tromey: you might want to test with IONFLAGS=logs and some test case executed with --ion-eager. [09:41] <nbp> tromey: this logging would be a good stress test for LSprinter. [09:42] I'll do this and report back here before re-requesting review.
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #11) > ::: commit-message-db9c7:1 > (Diff revision 2) > > +Bug 1334307 - do not allocate in GenericPrinter::vprintf; r?nbp > > remove this file. This is apparently something added by mozreview that will not actually land. It's definitely not in my patch here. So, just ignore it...
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Tom Tromey :tromey from comment #12) > From irc: > > <nbp> tromey: for Bug 1334307, I am not completely sure about the performance > concern. it might be that this is not a reall concern either. [09:40] > <firebot> https://bugzil.la/1334307 — NEW, ttromey@mozilla.com — > GenericPrinter::vprintf does not need to allocate > <nbp> tromey: you might want to test with IONFLAGS=logs and some test case > executed with --ion-eager. [09:41] > <nbp> tromey: this logging would be a good stress test for LSprinter. > [09:42] > > I'll do this and report back here before re-requesting review. I ran various things from jit-test like: IONFLAGS=logs /bin/time ./obj-x86_64-pc-linux-gnu/dist/bin/js --ion-eager js/src/jit-test/tests/sunspider/check-crypto-aes.js ... both with and without this patch, checking to make sure /tmp/ion.* was created. I didn't find any significant difference. However I'm not sure this is enough of a stress test. Is there a better test?
Flags: needinfo?(nicolas.b.pierron)
Comment 15•7 years ago
|
||
(In reply to Tom Tromey :tromey from comment #14) > IONFLAGS=logs /bin/time ./obj-x86_64-pc-linux-gnu/dist/bin/js --ion-eager > js/src/jit-test/tests/sunspider/check-crypto-aes.js > > ... both with and without this patch, checking to make sure /tmp/ion.* was > created. I didn't find any significant difference. > However I'm not sure this is enough of a stress test. Is there a better > test? This should be enough, thanks.
Flags: needinfo?(nicolas.b.pierron)
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8842525 [details] Bug 1334307 - do not allocate in GenericPrinter::vprintf; https://reviewboard.mozilla.org/r/116334/#review123860
Attachment #8842525 -
Flags: review?(nicolas.b.pierron) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
The try run pointed out that this used the wrong name in one spot (missing "js::") and that it should use "explicit" on the new constructor.
Comment 20•7 years ago
|
||
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/20318a73395e do not allocate in GenericPrinter::vprintf; r=nbp
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/20318a73395e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•