1886124 - Indentation of generated binding code is broken

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), defect)

Description

[Peter Van der Beken [:peterv]]

See for example https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/mozilla/dom/AbortControllerBinding.h#35

Comment 1

[Peter Van der Beken [:peterv]]

The patch's comment just says that 're.sub is overkill here', but it sure looks to me like the patch does change the behaviour of the indent() function to only indent the first line, as opposed to all the lines in the string.

Comment 2

[[:sergesanspaille]]

Attachment: Bug 1886124 - Fix regression on webidl indent while keeping part of the performance gain r=peterv!

According to ipython's %timeit:

>>> %timeit new_indent(a)
889 ns ± 5.54 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

>>> %timeit ref_indent(a)
2.65 µs ± 7.08 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

where ref_indent is the reference function, regexular-expression-based
new_indent is the one proposed in this patch

a is a small multiline string:

>>> a = """
... csrvrsvsr
...   csvfrz
... fvregvrg
... # frfger
...
... """

I've tested with

a = "errty" # small string

and

a = ("er" * 40 + '\n') * 100

and the split/join approach is always faster by a factor of at least 2.5

Comment 3

[[:sergesanspaille]]

My fault. I've submitted an updated patch and added you as a reviewer.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1884319

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbfd6042b4d2
Fix regression on webidl indent while keeping part of the performance gain r=peterv

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/dbfd6042b4d2

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:sergesanspaille, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox125 to wontfix.

For more information, please visit BugBot documentation.

Comment 8

[[:sergesanspaille]]

wontfix: it's not affecting the behavior of firefox, just making the source less convenient to read.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to [:sergesanspaille] from comment #8)

wontfix: it's not affecting the behavior of firefox, just making the source less convenient to read.

It does affect looking at crashes inside binding code on beta and release. It doesn't come up all that often but it also looks like a low risk patch.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Bug 1886124 - Fix regression on webidl indent while keeping part of the performance gain r=peterv

According to ipython's %timeit:

>>> %timeit new_indent(a)
889 ns ± 5.54 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

>>> %timeit ref_indent(a)
2.65 µs ± 7.08 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

where ref_indent is the reference function, regexular-expression-based
new_indent is the one proposed in this patch

a is a small multiline string:

>>> a = """
... csrvrsvsr
...   csvfrz
... fvregvrg
... # frfger
...
... """

I've tested with

a = "errty" # small string

and

a = ("er" * 40 + '\n') * 100

and the split/join approach is always faster by a factor of at least 2.5

Original Revision: https://phabricator.services.mozilla.com/D205061

Comment 11

[Phabricator Automation]

Uplift Approval Request

• User impact if declined: No direct impact. It will make understanding crashes inside generated binding code more difficult, which is not very common.
• Risk associated with taking this patch: Low
• Fix verified in Nightly: yes
• String changes made/needed: none
• Steps to reproduce for manual QE testing: none
• Code covered by automated testing: yes
• Explanation of risk level: It just changes indentation in generated bindings code.
• Is Android affected?: yes
• Needs manual QE test: no

Comment 12

[Phabricator Automation]

Uplift Approval Request

• User impact if declined: No direct impact. It will make understanding crashes inside generated binding code more difficult, which is not very common.
• Risk associated with taking this patch: Low
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Fix verified in Nightly: yes
• Code covered by automated testing: yes
• Explanation of risk level: It just changes indentation in generated bindings code.
• Is Android affected?: yes
• String changes made/needed: none

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0d5d481272b