Closed Bug 528651 Opened 15 years ago Closed 14 years ago

Component registrations not correctly cached leading to re-registering every component on every startup

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID
Tracking Status
blocking2.0 --- final+

People

(Reporter: mossop, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ts])

Attachments

(1 file)

I'm not sure why this isn't causing a Ts regression but since bug 511761 we aren't caching files in the component registry leading us to try re-register them on every startup unless you are running debug builds.
Assignee: nobody → dtownsend
Whiteboard: [ts]
Attached patch patch rev 1Splinter Review
We don't get a modification time for the module's file anymore except in debug builds so these tests are failing all the time. Seems like we only care about the modification time on debug builds too so there isn't much point in getting them and it is unclear when they might be 0 so this just caches the module's file regardless.

Pushing this to try to look at timings but this should be correct I think. At least I can now see files listed in compreg.dat.
Attachment #412346 - Flags: review?(benjamin)
Status: NEW → ASSIGNED
(In reply to comment #0)
> I'm not sure why this isn't causing a Ts regression

This is a really, really, really good question.  We have the registration and all that stuff there because it's "expensive" to do at startup -- so why are we being saved from that?
(In reply to comment #2)
> (In reply to comment #0)
> > I'm not sure why this isn't causing a Ts regression
> 
> This is a really, really, really good question.  We have the registration and
> all that stuff there because it's "expensive" to do at startup -- so why are we
> being saved from that?

One potential argument from this is that almost all of the components we are talking about here are JavaScript ones. Even though we would be loading them everytime their JS should be in the fastload so that should save us somewhat. I'm also looking and I'd actually guess that maybe half of them are used during startup anyway so for those we probably lose very little by re-registering them everytime.

That said I still think we should have seen some regression based on this, and more loss than the kind of win I'd have expected from the rest of bug 511761, but according to bug 511761 comment 33 we actually saw a win overall. I'm also not seeing very much interesting on the try talos runs from this, though the numbers are pretty messy to really draw any conclusions there.
Comment on attachment 412346 [details] [diff] [review]
patch rev 1

Wow, yes.
Attachment #412346 - Flags: review?(benjamin) → review+
Pushed: http://hg.mozilla.org/mozilla-central/rev/5795fd10ba3d

Will keep an eye on the talos results
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
I can't see any obvious signs of Ts improvement from this patch. In fact it is possible that it regressed Ts on windows but there are far too few data points to tell for sure at this point.

Just for fun I'm going to try pushing a patch that completely removes component caching to the try server to see what it does.
(In reply to comment #6)
> I can't see any obvious signs of Ts improvement from this patch. In fact it is
> possible that it regressed Ts on windows but there are far too few data points
> to tell for sure at this point.

It regressed MIN and MED dirty profile Ts on Vista, for about 2 and 3 percents, respectively.
As I would have hoped a patch that disables compreg.dat completely shows a healthy Ts regression, but this patch is certainly causing a regression too so I've backed it out for now. Deeper debugging is suggesting that there is still an issue here though.

http://hg.mozilla.org/mozilla-central/rev/0f5b586b91b6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 447581
Assignee: dtownsend → nobody
blocking2.0: --- → ?
blocking1.9.3final+
blocking2.0: ? → final+
We don't cache these at all anymore, due to changes in bug 568691.
Status: REOPENED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.