New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't zip AppliedTypeVar params and typeArgs #7592
Conversation
289fc5f
to
6646969
Compare
override def safeToString: String = super.safeToString + typeArgs.map(_.safeToString).mkString("[", ", ", "]") | ||
override def setInst(tp: Type): this.type = super.setInst { | ||
val instArgs = tp.typeArgs | ||
if (sameLength(zippedArgs, instArgs) && forall3(params, typeArgs, instArgs) { (param, targ, inst) => | ||
if (corresponds3(params, typeArgs, tp.typeArgs) { (param, targ, inst) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be inclined to keep using sameLength(params, tp.typeArgs)
, as a quick-fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes I think you're right! I was too excited to use corresponds3
.
I wonder if you ever want to use it actually 🤔 I found two places only:
if ((sym1 eq sym2) && (args1 ne Nil)) corresponds3(sym1.typeParams, args1, args2)(isSubArg) |
corresponds3(tps1, tps2, mapList(tparams)(_.variance))(isSubArg) |
Maybe there sameLength
would be better too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corresponds3
looks like misguided optimization. It's only used to check type parameters vs type arguments, but on average the number of those is not that large. Probably the subtyping checks are more expensive than a second traversal of the list. But on the other hand subtyping checks are optimized to handle common cases first. I wonder if we should try to:
- Change
corresponds3
to check the length first - Remove
corresponds3
altogether (I think this pattern ofsameLenght(args1, args2)
andforall3(args1, args2, params1)
is actually what you want - you don't need to checksameLength(args1, params1)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've changed here to reuse isSubArgs
because it makes the code cleaner. The fate of corresponds3
should be decided independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #7646
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My estimation, and understanding, which is however without benchmarks:
Most of the time, I would check length first. Since length only processes the list, not the elements, the worst it can do is to add a low-coefficient linear term (running through the list), which is easily subsumed by the main processing. Also, on the plus side, by checking length you are already prefetching all the list nodes (not its contents) up the cache-memory hierarchy, which you may want to.
In any case, this was a side matter, and I would have just left the code as it was on that. The main issue of this PR is, after all, to avoid creating extra lists.
Avoid unnecessary allocations.
6646969
to
1068769
Compare
Avoid unnecessary allocations.