Skip to content

a small handful of optimizations for 2.12#7135

Merged
retronym merged 6 commits intoscala:2.12.xfrom
hrhino:faster/misc
Oct 19, 2018
Merged

a small handful of optimizations for 2.12#7135
retronym merged 6 commits intoscala:2.12.xfrom
hrhino:faster/misc

Conversation

@hrhino
Copy link
Contributor

@hrhino hrhino commented Aug 25, 2018

Benchmarking results are here. I removed some commits which turned out to be counterproductive, so the hashes are wrong; the commit messages are correct, though. I'll run some more over the weekend.

cc @mkeskells @rorygraves

@hrhino hrhino added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Aug 25, 2018
@hrhino hrhino added this to the 2.12.7 milestone Aug 25, 2018
@hrhino

This comment has been minimized.

@hrhino hrhino changed the title [WIP] a small handful of optimizations for 2.12 [ci:last-only] [WIP] a small handful of optimizations for 2.12 Aug 25, 2018
@hrhino hrhino added the WIP label Aug 25, 2018
new ForeachPartialTreeTraverser(pf).traverse(this)
}

def changeOwner(pairs: (Symbol, Symbol)*): Tree = {
Copy link
Member

@retronym retronym Aug 28, 2018

Choose a reason for hiding this comment

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

Let's leave this version around as an overload, it is likely to be called from compiler plugins.

Literal(Constant(desc: String)),
Literal(Constant(required: Boolean))
), _) = sym.getAnnotation(LanguageFeatureAnnot).get
}
Copy link
Member

Choose a reason for hiding this comment

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

During the scala corpus benchmark, which has the language features defined in sources:

[error] 	Suppressed: java.util.NoSuchElementException: None.get
[error] 		at scala.None$.get(Option.scala:349)
[error] 		at scala.None$.get(Option.scala:347)
[error] 		at scala.reflect.internal.Symbols$LanguageFeature.<init>(Symbols.scala:3603)
[error] 		at scala.reflect.internal.Definitions$DefinitionsClass.getLanguageFeature(Definitions.scala:1222)
[error] 		at scala.reflect.internal.Definitions$DefinitionsClass$RunDefinitions.ImplicitConversionsFeature$lzycompute(Definitions.scala:1539)
[error] 		at scala.reflect.internal.Definitions$DefinitionsClass$RunDefinitions.ImplicitConversionsFeature(Definitions.scala:1539)
[error] 		at scala.tools.nsc.typechecker.Typers$Typer.typedDefDef(Typers.scala:2334)
[error] 		at scala.tools.nsc.typechecker.Typers$Typer.typedMemberDef$1(Typers.scala:5545)
[error] 		at scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5596)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... yikes. It bootstrapped just fine (obviously). I'll take a look as to why, or just get rid of the commit if it's not worth it.

Copy link
Member

Choose a reason for hiding this comment

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

The point of difference is that over in compiler-benchmark we jointly compile reflect/library/compiler sources, which seems to extend some weird corner cases with bootstrapping

@hrhino
Copy link
Contributor Author

hrhino commented Sep 4, 2018

Will come back to this today.

@adriaanm
Copy link
Contributor

@hrhino ping

@adriaanm adriaanm modified the milestones: 2.12.7, 2.12.8 Sep 17, 2018
@hrhino
Copy link
Contributor Author

hrhino commented Oct 3, 2018

one of these is not pulling its weight. looking into it, then I'll rebase.

(sorry for the extended absence, I've been really busy sorting out job stuff.)

@adriaanm
Copy link
Contributor

I was passing by, so decided to take the tedium out of it for you. Hope you don't mind my push -f (from 9614ee2 to a227295). I restored the changeOwner overload, and moved the dubious commit last.

@hrhino
Copy link
Contributor Author

hrhino commented Oct 18, 2018

Thanks, @adriaanm.

I removed the dubious commit (although I still can't get it to fail on the benchmark) as not worth the effort. I reran the benchmarks which show that b2c493d is most of the improvement.

I'm gonna take the WIP label off. Let me know what you think.

@hrhino hrhino removed the WIP label Oct 18, 2018
@hrhino hrhino changed the title [WIP] a small handful of optimizations for 2.12 a small handful of optimizations for 2.12 Oct 18, 2018
@retronym

This comment has been minimized.

@retronym retronym merged commit ebf8017 into scala:2.12.x Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance the need for speed. usually compiler performance, sometimes runtime performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants