GenericPrinter::vprintf does not need to allocate

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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

11 months 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.
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

11 months 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)
(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)

Updated

11 months ago
Depends on: 1343292
(Assignee)

Comment 5

11 months ago
This is a bit cleaner to do once bug 1341880 is fixed.
Depends on: 1341880
(Assignee)

Comment 6

11 months ago
I've also changed the code to remove some now-redundant overrides.
Comment hidden (mozreview-request)
(Assignee)

Comment 8

11 months ago
I'll add the r? once the blocker lands.
Assignee: nobody → ttromey
(Assignee)

Comment 9

11 months ago
The blocker is r+ and in try so I'll reattach this patch.
Comment hidden (mozreview-request)

Comment 11

11 months 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

11 months 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

11 months 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

10 months 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)
(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

10 months 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

10 months 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

10 months ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20318a73395e
do not allocate in GenericPrinter::vprintf; r=nbp

Comment 21

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20318a73395e
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

10 months ago
Depends on: 1350606
Depends on: 1350097
You need to log in before you can comment on or make changes to this bug.