Closed Bug 1334307 Opened 7 years ago Closed 7 years ago

GenericPrinter::vprintf does not need to allocate

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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.
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".
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)
Depends on: 1343292
This is a bit cleaner to do once bug 1341880 is fixed.
Depends on: 1341880
I've also changed the code to remove some now-redundant overrides.
I'll add the r? once the blocker lands.
Assignee: nobody → ttromey
The blocker is r+ and in try so I'll reattach this patch.
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)
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.
(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...
(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 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+
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.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20318a73395e
do not allocate in GenericPrinter::vprintf; r=nbp
https://hg.mozilla.org/mozilla-central/rev/20318a73395e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1350606
Depends on: 1350097
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: