Skip to content

Conversation

@nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

dottybot commented Apr 6, 2018

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


/** perform context-dependant initialization */
override def prepareForUnit(tree: Tree)(implicit ctx: Context) = {
primitiveGetClassMethods = Set[Symbol]() ++ defn.ScalaValueClasses().map(x => x.requiredMethod(nme.getClass_))
Copy link
Contributor

@biboudis biboudis Apr 6, 2018

Choose a reason for hiding this comment

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

So, the improvement is that before prepareForUnit returned all the symbols for getClass methods, by generating stubsymbols were needed, and now it doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Mostly the cost was in creating the set.

@dottybot
Copy link
Member

dottybot commented Apr 6, 2018

Performance test finished successfully:

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

Benchmarks is based on merging with master (2f94916)

@smarter smarter removed their assignment Apr 6, 2018
}*/
*/
case t if primitiveGetClassMethods.contains(t) =>
case t if t.name == nme.getClass_ && scalaValueClasses.contains(t.owner) =>
Copy link
Member

Choose a reason for hiding this comment

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

I think you could just do defn.ScalaValueClasses.contains, no need for the intermediate variable.

@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

dottybot commented Apr 6, 2018

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

@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented Apr 6, 2018

There is some performance regression on some of the exhaustivity benchmarks. I will investigate before merging.

@nicolasstucki nicolasstucki self-assigned this Apr 6, 2018
@dottybot
Copy link
Member

dottybot commented Apr 6, 2018

Performance test finished successfully:

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

Benchmarks is based on merging with master (2f94916)

@nicolasstucki
Copy link
Contributor Author

Actually the performance regression was on master.

@nicolasstucki nicolasstucki merged commit af7bde7 into scala:master Apr 6, 2018
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.

4 participants