Closed Bug 851342 Opened 11 years ago Closed 11 years ago

More Spidermonkey builds for generational garbage collection

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(4 files)

Miss me? :-)

Generational garbage collection (GGC) is getting close to landing, and we're eventually going to want to add some builds checking whether we've broken it. We're not quite there yet, but there are a couple of try builds that would be handy for other people to use to check if they're breaking GGC.

I am adding them as nondefault for now, to avoid overhead and confusion while they're not quite real yet. This has admittedly rather odd semantics: |-p all| will not get them, |-p linux64| will. Really, something like |-b exactrooting -p all| should work instead, or perhaps some sort of project-specific syntax. But these only fire for changes touching js/src in the first place, so I don't think load should be a problem.

Note that it is currently unclear whether we will later want to promote these to be default builds on try, and/or which of these we'll want to put onto inbound. It depends on how quickly different things progress -- eg, we may end up merging rootanalysis and exactrooting, and if ggc turns out to be stable it'll get folded in too. Eventually, this will boil back down to a single extra build, I think. But for now, there are a couple of very different things that random JS commits can break.

My guess is that I'll want to turn these on for try by default as soon as they're more or less green. I'm not even sure how much having them off by default is going to save, given the wacky semantics.
As an extra trick, this patch also allows pushing with a file js/src/config.try to override the configure settings. That's because apparently some of these are currently only expected to work with the JITs off, and I don't want to have to update these files constantly to match the current state of the code. This patch contains the "final" versions to be used eventually.
Attachment #725154 - Flags: review?(bhearsum)
Sadly, I'll need to create new config chunks for these different builds to make some of them nondefault on try. Which means that my scheduler name can't only depend on the branch name.

Actually, there's another reason why these builds have to have their own config chunks. More on that in a later patch.
Attachment #725155 - Flags: review?(bhearsum)
I changed dashes to underscores for other parts of the config, but neglected to make the change for spidermonkey builds. This should affect no current builds, since no spidermonkey builds use try_by_default yet.
Attachment #725156 - Flags: review?(bhearsum)
Add in the configuration for the new builds.

Unhappily, each build needs its own project config chunk, because they are on the try branch and go through tryChooser. tryChooser's prettyNames only allows a single builder per platform, so if you have multiple entries in the 'variants' list it'll just use one of them.

Cleaning that up would require changing some of the basic data structures that tryChooser uses, which feels like another bug. (I think it would be a good change even without these new builds, though. I'm hoping it will be possible to unify the data types between build and test prettyNames, which would simplify try_parser a fair amount.)
Attachment #725158 - Flags: review?(bhearsum)
Comment on attachment 725158 [details] [diff] [review]
Add new try-only spidermonkey builds that are off by default

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

::: mozilla/config.py
@@ +1107,5 @@
> +    'spidermonkey_ggc_try': {
> +        'enable_try': True,
> +        'try_by_default': False,
> +        'variants': {
> +            'linux64-debug':  ['generational'],

It strikes me that the only differences between this and spidermonkey_try are the variants and try_by_default. Maybe try_by_default should be overridable or solely configurable at the variant level, then we don't need these extra projects?
(In reply to Ben Hearsum [:bhearsum] from comment #5)
> Comment on attachment 725158 [details] [diff] [review]
> Add new try-only spidermonkey builds that are off by default
> 
> Review of attachment 725158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozilla/config.py
> @@ +1107,5 @@
> > +    'spidermonkey_ggc_try': {
> > +        'enable_try': True,
> > +        'try_by_default': False,
> > +        'variants': {
> > +            'linux64-debug':  ['generational'],
> 
> It strikes me that the only differences between this and spidermonkey_try
> are the variants and try_by_default. Maybe try_by_default should be
> overridable or solely configurable at the variant level, then we don't need
> these extra projects?

Yes and no. First, I agree. But it is going to require some substantial changes. Simply making try_by_default per-variant is very straightforward, but sadly it wouldn't work because there's a completely different reason why these have to be separate projects regardless of the try_by_default setting: for builds, tryChooser only allows a single builder per platform. prettyNames is a mapping from a platform to a single builder name. (For tests, prettyNames maps to a richer data structure, but unfortunately these are builds.) If I have multiple variants in any enable_try project build, it will end up only triggering one of them.

I haven't tried yet, but I think it would be a pretty deep change to try_parser.py to change the interpretation of build prettyNames.

That said, I plan to make that change, but I don't have time now and wanted to get something straightforward in that works first. Would that be acceptable? I can file a followup bug if so. (I will probably also want to discuss it with you or someone first, since I'd like to unify the type and handling of prettyNames across builds and tests if possible rather than just making them be different in a different way. I'll rename it too.)
(In reply to Steve Fink [:sfink] from comment #6)
> (In reply to Ben Hearsum [:bhearsum] from comment #5)
> > Comment on attachment 725158 [details] [diff] [review]
> > Add new try-only spidermonkey builds that are off by default
> > 
> > Review of attachment 725158 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mozilla/config.py
> > @@ +1107,5 @@
> > > +    'spidermonkey_ggc_try': {
> > > +        'enable_try': True,
> > > +        'try_by_default': False,
> > > +        'variants': {
> > > +            'linux64-debug':  ['generational'],
> > 
> > It strikes me that the only differences between this and spidermonkey_try
> > are the variants and try_by_default. Maybe try_by_default should be
> > overridable or solely configurable at the variant level, then we don't need
> > these extra projects?
> 
> Yes and no. First, I agree. But it is going to require some substantial
> changes. Simply making try_by_default per-variant is very straightforward,
> but sadly it wouldn't work because there's a completely different reason why
> these have to be separate projects regardless of the try_by_default setting:
> for builds, tryChooser only allows a single builder per platform.
> prettyNames is a mapping from a platform to a single builder name. (For
> tests, prettyNames maps to a richer data structure, but unfortunately these
> are builds.) If I have multiple variants in any enable_try project build, it
> will end up only triggering one of them.
> 
> I haven't tried yet, but I think it would be a pretty deep change to
> try_parser.py to change the interpretation of build prettyNames.
> 
> That said, I plan to make that change, but I don't have time now and wanted
> to get something straightforward in that works first. Would that be
> acceptable? I can file a followup bug if so.

Sounds good to me.
Attachment #725154 - Flags: review?(bhearsum) → review+
Attachment #725155 - Flags: review?(bhearsum) → review+
Attachment #725156 - Flags: review?(bhearsum) → review+
Comment on attachment 725158 [details] [diff] [review]
Add new try-only spidermonkey builds that are off by default

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

::: mozilla/config.py
@@ +1092,5 @@
>          'repo_path': 'projects/nanojit-central',
>      },
>      'spidermonkey_try': {
>          'enable_try': True,
> +        'try_by_default': True,

Based on my reading of misc.py it looks like this just making try_by_default be an explicit setting instead of relying on the default in misc.py. r=me assuming that's correct.
Attachment #725158 - Flags: review?(bhearsum) → review+
(In reply to Ben Hearsum [:bhearsum] from comment #8)
> Comment on attachment 725158 [details] [diff] [review]
> Add new try-only spidermonkey builds that are off by default
> 
> Review of attachment 725158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozilla/config.py
> @@ +1092,5 @@
> >          'repo_path': 'projects/nanojit-central',
> >      },
> >      'spidermonkey_try': {
> >          'enable_try': True,
> > +        'try_by_default': True,
> 
> Based on my reading of misc.py it looks like this just making try_by_default
> be an explicit setting instead of relying on the default in misc.py. r=me
> assuming that's correct.

Yes, that's correct. When reading the three together, it just felt like having try_by_default: False for two of them and not mentioning it for the other was more confusing.
Blocks: 852618
Attachment #725154 - Flags: checked-in+
Attachment #725155 - Flags: checked-in+
Attachment #725156 - Flags: checked-in+
Merged change(s) from this bug and I reconfigured the masters.
(In reply to Armen Zambrano G. [:armenzg] (Release Enginerring) from comment #11)
> Merged change(s) 

So this bug can be closed?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: