Skip to content
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

Rework modifyAll for Scala 3 #82

Merged
merged 6 commits into from
Mar 9, 2022

Conversation

KacperFKorban
Copy link
Collaborator

This PRa attempts to rework the way modifyAll method is generated in Scala 3.
ATM a call like

bob.modifyAll(_.age, _.height).setTo(1)

is equivalent to

bob.modify(_.age).setTo(1).modify(_.height).setTo(1)

and generates

bob.copy(age = 1).copy(height = 1)

The bytecode from this method is suboptimal.

The proposed approach relies on compressing all paths into a paths tree and generating code from that.
This allows to generate the following code for the given example:

bob.copy(age = 1, height = 1)

For example, for an invocation of the modifyAll from RepeatedModifyAllTest in an empty Main class, the bytecode sizes are:

  • for Scala 3 with path compressing -- 440 lines
  • for Scala 2 -- 4510

This PR is a draft since it is not very polished and needs a lot more tests.
Though I would like to know your opinion about it first. cc @adamw

@adamw
Copy link
Member

adamw commented Feb 25, 2022

I didn't read the code (just skimmed), but if you can make it work, I'll be of course happy to merge :)

The main challenge will be grouping the fields by common modification path prefix, so that the combined copies are done when possible. So when you have x.modifyAll(_.a.b, _.a.c, _.d) you generate x.copy(a = x.a.copy(b = ..., c = ...), d = ...). But you probably though about that :)

@KacperFKorban
Copy link
Collaborator Author

I think that it's ready now. Added tests for all edge cases I could think of.


An interesting example to demonstrate the order of modification:

case class Cons(head: Int, tail: Option[Cons])
val sth = Cons(1, Some(Cons(2, None)))
sth.modifyAll(_.tail, _.tail.each.tail).using { x =>
  println(x)
  x
}

The above code will print:

Some(Cons(2,None))
None

but the following code:

sth.modifyAll(_.tail.each.tail, _.tail).using { x =>
  println(x)
  x
}

Will output:

None
Some(Cons(2,None))

This demonstrates the following rule:

forall paths A and B:
  if (A is a prefix of B or B is a prefix of A) then
    modification of A will always be performed before the modification of B.

One potential improvement for the future would be that comparing the equivalence/equality of function delegates is a non-trivial problem. In our case, it implies that for now, calls to e.g. each in paths will be correctly optimized, but calls to e.g. eachWhere will not be optimized and will generate two eachWhere calls (instead of the optimal one). This is because the semantic equivalence problem for functions is undecidable. This could be further improved in the future with some smart heuristics like some syntactical equality checks etc.

@KacperFKorban KacperFKorban marked this pull request as ready for review March 5, 2022 17:56
@KacperFKorban KacperFKorban requested a review from adamw March 5, 2022 17:56
@adamw
Copy link
Member

adamw commented Mar 9, 2022

Indeed, an impressive test suite - thanks! :)

1 similar comment
@adamw
Copy link
Member

adamw commented Mar 9, 2022

Indeed, an impressive test suite - thanks! :)

@adamw adamw merged commit fdae07d into softwaremill:master Mar 9, 2022
@KacperFKorban KacperFKorban deleted the rework-modifyAll branch March 9, 2022 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants