Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
In bug 1161377 I tried using a flag to distinguish PLDHashTables that use manual initialization/finalization from those that use automatic init/fini.

Thinking some more, I think it's better to introduce a new subclass of PLDHashTable (imaginatively called PLDHashTable2) that contains an initializating constructor and destructor, and forbids the use of the manual Init()/Fini() methods. This will allow existing PLDHashTable instances to be converted incrementally, which hopefully will help avoid Android regressions. (E.g. I won't be suddenly adding lots of no-op destructor calls all in one hit.)

My plan is to eventually convert all the existing PLDHashTable instances to this new form, and then merge PLDHashTable2 into PLDHashTable. I've looked closely at all the existing instances and I'm confident this is possible without too much difficulty.
(Assignee)

Updated

4 years ago
Blocks: 1165768
(Assignee)

Updated

4 years ago
Blocks: 1165786
(Assignee)

Comment 1

4 years ago
Created attachment 8606883 [details] [diff] [review]
Add PLDHashTable2

This is a temporary sub-class of PLDHashTable that will allow PLDHashTable to
be incrementally transitioned from manual initialization/finalization (via
explicit Init()/Fini() calls) to automatic initialization/finalization (via an
initializing constructor and a destructor). Once all PLDHashTable instances are
converted to PLDHashTable2, it can be folded back into PLDHashTable and the "2"
suffix can be dropped.
Attachment #8606883 - Flags: review?(nfroyd)
Comment on attachment 8606883 [details] [diff] [review]
Add PLDHashTable2

Review of attachment 8606883 [details] [diff] [review]:
-----------------------------------------------------------------

I'm guessing the next steps are something like:

1) Land this patch;
2) Land one (or more, if feeling confident) patches from bug 1165768, watch Android autophone benchmarks.
3) Try to figure out how to address regressions, if any.
4) Goto step 2 until no more patches remain.

?  What happens if we just get stuck at step 3, i.e. things regress, but there doesn't seem to be any good reason?  Back everything out?

Equally, what's the benefit of having this patch stack on m-c versus you spending quality time with Try?  Do we simply not have the applicable benchmarks available on try, and landing things on m-c is the only way to get things tested?  (That would be a sorry state of affairs if so...)

::: xpcom/glue/pldhash.h
@@ +342,5 @@
> +// overrides the Init() and Finish() methods with versions that will crash
> +// immediately if called.
> +//
> +// XXX: Eventually, all instances of PLDHashTable will be replaced with
> +// PLDHashTable2 and the latter will be renamed as the former.

Maybe add something along the lines of "We're using a subclass here so that instances of PLDHashTable can be converted over incrementally"?
(Assignee)

Comment 3

4 years ago
> I'm guessing the next steps are something like:
> 
> 1) Land this patch;
> 2) Land one (or more, if feeling confident) patches from bug 1165768, watch
> Android autophone benchmarks.
> 3) Try to figure out how to address regressions, if any.
> 4) Goto step 2 until no more patches remain.

Yes.

> ?  What happens if we just get stuck at step 3, i.e. things regress, but
> there doesn't seem to be any good reason?  Back everything out?
> 
> Equally, what's the benefit of having this patch stack on m-c versus you
> spending quality time with Try?  Do we simply not have the applicable
> benchmarks available on try, and landing things on m-c is the only way to
> get things tested?  (That would be a sorry state of affairs if so...)

Assorted thoughts...

- I've done some Android try pushes to check for perf regressions. Results are partially in and looking fine so far. I plan to keep doing them.

- In the past when I've had trouble landing changes like this, redoing them in smaller pieces has always worked well.

- I suspect the Android regressions were kind of random and that landing basically the same code in a different way will avoid problems. If that is false, landing things in smaller pieces will make it easier to identify the cause of regressions. I am being optimistic that these perf regressions will be resolvable; in the end it's just refactoring.

- I get nervous maintaining large stacks of patches. I currently have 20 and there's probably going to be at least another 10 before PLDHashTable2 can go away.

If you're still worried I can take the big bang approach and do all the necessary patches at once. There will probably end up being 30+ in a single bug.
(Assignee)

Comment 4

4 years ago
Thinking some more...

This PLDHashTable refactoring stuff has proven fiddly. (See bug 1050035 and bug 1161377 -- I've removed and then unremoved PL_NewDHashTable() and PL_DHashTableDestroy() twice now.) There have been correctness and performance issues. The correctness issues scare me more.

If I land 30+ patches at once and a subtle problem shows up -- e.g. some mochitest starts failing intermittently a day later, it could be very hard to work out what's going on. If I land a few patches at a time, spread out over a period of weeks, it makes dealing with that situation a lot easier.

I guess I could satisfy both of us by writing all the patches at once, getting r+ on them, but then landing them gradually. That does increase the likelihood of rebasing problems, though they probably won't be bad in this case.
(Assignee)

Comment 5

4 years ago
The incremental approach also eases your reviewing burden, by spreading the requests out instead of dumping a whole pile on at once.
Comment on attachment 8606883 [details] [diff] [review]
Add PLDHashTable2

Review of attachment 8606883 [details] [diff] [review]:
-----------------------------------------------------------------

OK, all of this makes sense.  I'm beginning to wonder if there was some small amount of extra work being done in the constructor(s) you introduced in other portions of the code--initializing members or something like that--that caused the regressions.  Ideally landing things in small chunks will show things like that.
Attachment #8606883 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/8aa33b2cd1b1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.