Skip to content

Various fixes related to inlining #4464

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

Merged
merged 7 commits into from
May 8, 2018

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 4, 2018

No description provided.

odersky added 5 commits May 4, 2018 19:15
In readLater the context is not preserved, but reconstituted using the
owner of the context building the closure. We need to pass the mode as
well, or else ReadPositions might be reset. I noticed this when I saw
that positions were not read correctly when inlining.
…nitions

This was done by accident before, because we did not keep the mode bits
of the context calling a readLater.
When printing trees, modifiers were always taken from the tree. This is wrong
for typed trees, since their flags and annotations come from the symbol, the
tree modifiers are no longer relevant.
Was forgotten before, which meant that labels were forgotten in inlined
code. The code was still correct, but a method would be created for each label.
odersky added 2 commits May 5, 2018 16:32
The idea is to have a compiler-specific list data type that
optimizes for small lists. Where the overhead of normal lists
is one Cons node per element, the overhead of `Lst` is
0 for lists of length <= 1, and 1 array node for longer lists.
Furthermore, we'll try to inline heavily, which is easier for
specialized data structures.

The main downsides are inefficient `tail` and `::`, in particular
for longer lists. Compiler code will need to be adapted to avoid these where
it is performance critical.

This also serves as test case for the inline related fixes in the
previous commits.
@odersky
Copy link
Contributor Author

odersky commented May 6, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented May 6, 2018

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

dottybot commented May 6, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4464/ to see the changes.

Benchmarks is based on merging with master (6f6816d)

@nicolasstucki
Copy link
Contributor

Are all the fixes tested by tests/run/lst/LstTest.scala?

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to do proper benchmarks to compare with normal lists and plain arrays.


def length: Int = elems match {
case null => 0
case elems: Arr => elems.length
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not behave correctly if we have Lst[Array[Any]].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for every other pattern match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an updated version of Lst in the "Try some Optimizations" pull request that handles this case. The version in this PR serves just as a test case for the inliner.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

def sharedOp(x: T) = op(x)
elems match {
case null =>
case elems: Arr => def elem(i: Int) = elems(i).asInstanceOf[T]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this cast in a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an automatic rewrite, to avoid having to cast at multiple places in the body. I rewrote:

case elems: Array[T] =>

to what you see here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it makes sense :)

private def eq(x: Any, y: Any) = x.asInstanceOf[AnyRef] `eq` y.asInstanceOf[AnyRef]
private val eqFn = (x: Any, y: Any) => eq(x, y)

val Empty = new Lst[Nothing](null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not an inline def? That will just be null in the bytecode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll update it in the newer version of Lst.

@odersky
Copy link
Contributor Author

odersky commented May 8, 2018

Are all the fixes tested by tests/run/lst/LstTest.scala?

yes.

@odersky odersky assigned nicolasstucki and unassigned odersky May 8, 2018
@nicolasstucki nicolasstucki removed their assignment May 8, 2018
@nicolasstucki nicolasstucki merged commit 76a8ccb into scala:master May 8, 2018
@allanrenucci allanrenucci deleted the fix-inlines branch May 8, 2018 15:47
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.

3 participants