Retry only generating cask variations for supported macOS configurations

[Rylan12]

Retry #20080

To be honest, I’m still not entirely sure what went wrong, so I would encourage others to investigate this more. I tried using brew prof, but wasn’t able to get particularly useful info (although this is probably me not using it properly).

---

Here is what I found. For some reason, it seems like the generate-cask-api job hangs when processing the ipartition cask. As far as I can tell, there doesn’t seem to be anything particularly unique about that cask. It only works on macOS versions up to High Sierra, and while it does have a variation block, the depends_on line is outside.

If I manually run :ipartition.c.to_hash_with_variations, it seems to work just fine without hanging. Similarly, if I modify brew generate-cask-api to skip all casks except ipartition, it does not hang. However, when I modify brew generate-cask-api to skip all casks that don’t start with the letter i, it does hang. If I modify brew generate-cask-api to include all casks except ipartition, it doesn’t seem to hang.

From this, it seems like there is something special about the ipartition cask that causes the generation to hang, but only when processed with others. There is definitely more investigating to be done.

I ended up determining that the line that hands is inside MacOSRequirement#allows?, where two MacOSVersion objects are compared using <=:

brew/Library/Homebrew/requirements/macos_requirement.rb

Line 83 in d428e83

when "<=" then other <= @version

This comparison is reached many times per-cask, and for some reason, when processing ipartition, this line gets slower and slower each time. By the time we are comparing 11 to 10.13 (the latter being the maximum version allowed by the depends_on, we’re already pretty slow. By the time we get to comparing 10.14 to 10.13, I wasn’t able to run the entire comparison before I gave up and exited (which took at least 10 minutes).

If we look at the MacOSVersion#<=> method, it caches each comparison to avoid needing to compute the comparisons over and over again:

brew/Library/Homebrew/macos_version.rb

Lines 64 to 82 in d428e83

	def <=>(other)
	return @comparison_cache[other] if @comparison_cache.key?(other)
	result = case other
	when Symbol
	if SYMBOLS.key?(other) && to_sym == other
	0
	else
	v = SYMBOLS.fetch(other) { other.to_s }
	super(v)
	end
	else
	super
	end
	@comparison_cache[other] = result unless frozen?
	result
	end

Since the cache key is a MacOSVersion object, I thought maybe the issue was that we were recursively comparing MacOSVersions which was causing the issue. I also thought maybe we were creating new MacOSVersion objects each time, and these weren’t comparing properly.

So, I made two changes to MacOSVersion that are included in this PR:

1. Add caching to ::from_symbol so the same object is returned each time a symbol is requested
2. In #<=>, use the stringified version of the other object being compared as the cache key to make the includes? check a simple string comparison

Both of these solutions seemed to work on their own to solve this, and I was able to run brew generate-cask-api in ~30 seconds on my machine (compared to longer than 10 minutes before, I never waited long enough to know if it actually returned).

---

Clearly, this is weird and I think I’ve gone a little too far down the rabbit hole to really know what the best solution is, so I would appreciate extra eyes from anyone who's willing to do some investigating

I don't really want to merge this as-is because my gut tells me there is either a better way to handle this all together or something else is the root of the problem

[carlocab]

Seems fine to me TBH.

Caching .from_symbol makes perfect sense.

I'm a little bit more suspicious about stringify-ing keys in @comparison_cache, since these can be T.untyped and I don't know if the string representation captures enough of what we care about. But it's probably fine.

[carlocab]

I wonder if we're comparing frozen objects too often, which prevents caching here. Maybe we should make the comparison_cache belong to the class, instead of the instance?

[MikeMcQuaid]

Yeh, feels similar to me: this frozen? check seems nice to be able to avoid if possible.

[MikeMcQuaid]

Can't tell you where off the top of my head but we definitely have other classes where we use Cachable on the class instead of the instance.

[Rylan12]

Yeah, actually, the from_symbol cache that I added is class-wide instead of on the instance, and I totally could move this to live there too (which is probably better anyway).

Why does having the cache belong to the class instead of the instance make a difference for frozen objects?

[MikeMcQuaid]

If you've reproduced the bug and this fixes things: fine with me to merge as-is and then iterate.

Thanks for this!

[MikeMcQuaid]

Yeh, feels similar to me: this frozen? check seems nice to be able to avoid if possible.

[MikeMcQuaid]

I've definitely been seeing some messages like this:

The class ... reached 8 shape variations, instance variables accesses will be slower and memory usage increased.
It is recommended to define instance variables in a consistent order, for instance by eagerly defining them all in the #initialize method.

I wonder if that could be related and improve performance here too?

A nice way of handling that is making a file typed: strict which will nudge you to do this.

[Rylan12]

I'm a little bit more suspicious about stringify-ing keys in @comparison_cache, since these can be T.untyped and I don't know if the string representation captures enough of what we care about. But it's probably fine.

Yep, that's a valid concern. I checked this with versions like "10.15", "11", :big_sur, etc and it seems fine, but I didn't check this with all of the other types of Version objects that we can have, so maybe I need to look into that more.

I've definitely been seeing some messages like this:

The class ... reached 8 shape variations, instance variables accesses will be slower and memory usage increased.
It is recommended to define instance variables in a consistent order, for instance by eagerly defining them all in the #initialize method.

I wonder if that could be related and improve performance here too?

A nice way of handling that is making a file typed: strict which will nudge you to do this.

Yeah, I've seen this a bunch too. I'll take a look

[samford]

A nice way of handling that is making a file typed: strict which will nudge you to do this.

I have a local branch where (among other things) I took care of all of the type changes to upgrade MacOSVersion to typed: strong. I don't think that commit depends on anything else in that branch, so I'll see about pulling it out and creating a PR for it (in case it would be helpful for this PR).

[Bo98]

This definitely isn't correct as it assumes everything is compared on String level which is not true:

MacOS.version == :sequoia
=> true
MacOS.version == "sequoia"
=> false

[Bo98]

A compromise could be to special case this to version objects since we can assume they are compared equivalently to strings.

cache_key = other.is_a?(Version) ? other.to_s : other

This should keep the optimisation you have here while not regressing the above example.

---

Out of interest, I checked to see where the comparison cache is needed. A lot of the time spent is runtime type checking, which we could disable for version.rb comparison methods (while keeping static type checking) given the runtime type safety there is not particularly useful anyway due to the T.untyped nature of <=>. Comparison cache will always be quicker given it's simply less instructions, but you can make the uncached case over 5x faster by reducing type checks.

[Rylan12]

I have a local branch where (among other things) I took care of all of the type changes to upgrade MacOSVersion to typed: strong. I don't think that commit depends on anything else in that branch, so I'll see about pulling it out and creating a PR for it (in case it would be helpful for this PR).

@samford that would be great

[samford]

The PR to upgrade MacOSVersion to typed: strong and expand test coverage to 100% (for lines and branches) is now up at #20108. This PR appears to rebase fine on that one but you will need to resolve some brew typecheck errors related to T.untyped values. Shout if you need any help with that but if it ends up being a problem, we can always drop MacOSVersion down to typed: strict if needed.