1116524 - 7.3% regression on ss-unpack-code when enabling tracelogger

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

See AWFY. There are also others: 
- 5.8% on ss-tagcloud
- 7.3% on ss-unpack-code
- 4.9% on octane-regexp

Comment 1

[Hannes Verschore [:h4writer]]

- 156.4% on kraken-fft

Comment 2

[Nicholas Nethercote [inactive]]

Given these regressions and the increase in static initializers, should the tracelogger-is-always-on patch be backed out for now?

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Given these regressions and the increase in static initializers, should the
> tracelogger-is-always-on patch be backed out for now?

I'm still carefully investigating the regressions. But it is very hard. Currently it still looks like a timing issue and very sensitive to the machine. I haven't been able to reproduce on another computer and on the same the computer it is also hard to reproduce. Using the harness I get a regression. Manually running it, doesn't give me a regression... So a very small shift that causes these regression, which are unfortunate, but actually not really caused by tracelogger itself. (TL is included by default, but not enabled). Still investigating!

Also it wouldn't be smart to backout everything. It might just be good enough to create a patch that switch the configure option again so you need to configure tracelogger in a build. If it takes too long that is definitely an option I'm considering.

About the static initializers. I didn't know they were that bad? Would it be a stopgap into keeping it included by default? Just wanting to know how severe it is, so I can prioritize it. I think finding/fixing the performance issues have higher priority atm, right?

Comment 4

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> Also it wouldn't be smart to backout everything. It might just be good
> enough to create a patch that switch the configure option again so you need
> to configure tracelogger in a build. If it takes too long that is definitely
> an option I'm considering.

I'm not coming one inch closer to finding the reason for the regression. I'll give myself tomorrow and if I didn't find the issue till tomorrow night, I'll land a patch to not include tracelogging by default.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> 
> About the static initializers. I didn't know they were that bad? Would it be
> a stopgap into keeping it included by default? Just wanting to know how
> severe it is, so I can prioritize it. I think finding/fixing the performance
> issues have higher priority atm, right?

I know that froydnj and glandium have spent considerable effort getting rid of static initializers in the past. IIRC it can hurt start-up time.

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> (In reply to Hannes Verschore [:h4writer] from comment #3)
> > 
> > About the static initializers. I didn't know they were that bad? Would it be
> > a stopgap into keeping it included by default? Just wanting to know how
> > severe it is, so I can prioritize it. I think finding/fixing the performance
> > issues have higher priority atm, right?
> 
> I know that froydnj and glandium have spent considerable effort getting rid
> of static initializers in the past. IIRC it can hurt start-up time.

Ok thanks. I think I should be able to create them dynamically. I'll do that in a follow-up bug.

As for the regression. They seemed to be an alignment issue. We know we have a lot of them. Bbouvier is currently busy fixing some. Which is nice. But this won't fix this issue. I still need to decide what the best solution is.
- Aligning to 32 bytes instead of 16 bytes. 
- Find out what knocked the alignment in my patches. Though this might make the regression disappear but not totally fix the issue. (I have a feeling this is more accidental and that it might be possible to adjust the stack position outside "JS" to again be at the wrong offset if we don't start aligning on 32 bytes). Though I'm quite close to finding that: "https://hg.mozilla.org/mozilla-central/rev/7091b8b54c91" is the culprit. 

So I'm close now, finally :D.

Comment 7

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

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.