Skip to content

Fix ensureFreshScopeAfter always cloning scope #3222

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 1 commit into from
Oct 1, 2017

Conversation

smarter
Copy link
Member

@smarter smarter commented Oct 1, 2017

PR on top of #3216, only the last commit is new.

@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

test performance please

@dottybot
Copy link
Member

dottybot commented Oct 1, 2017

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

@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

Oops, I should have tested commit N-1 and N to get useful information. @liufengyun is there a way to cancel a running test?

@liufengyun
Copy link
Contributor

You can issue another test for N - 1: test performance please: 196c23c --- the results will accumulate.

@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

Yes but they won't be in the correct order which will look confusing :).

@liufengyun
Copy link
Contributor

Yes but they won't be in the correct order which will look confusing

The hash will help disambiguate :)

I manually cancelled the job, you can reissue the command. I'll add a command to cancel running jobs.

@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

Thanks!

@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

test performance please: 196c23c ea1400f

@dottybot
Copy link
Member

dottybot commented Oct 1, 2017

performance test scheduled for 196c23c ea1400f: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

dottybot commented Oct 1, 2017

performance test failed:

[check /data/workspace/bench/logs/pull-3222-10-01-16.08.out for more information]

@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

@liufengyun help 😄 ^

@liufengyun
Copy link
Contributor

test performance please: 196c23c ea1400f

There's some issue with the lock of jmh when force killing test.

@dottybot
Copy link
Member

dottybot commented Oct 1, 2017

performance test scheduled for 196c23c ea1400frnrnheres some issue with the lock of jmh when force killing test: 1 job(s) in queue, 0 running.

@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

ouch, bad parsing :P

@liufengyun
Copy link
Contributor

test performance please: 196c23c ea1400f

@dottybot
Copy link
Member

dottybot commented Oct 1, 2017

performance test scheduled for 196c23c ea1400frn: 1 job(s) in queue, 0 running.

@liufengyun
Copy link
Contributor

There's some regression with the multi-commits test -- I just manually scheduled the test.

@liufengyun
Copy link
Contributor

The multi-commit command should work now with the fix (already deployed on server): liufengyun/bench@259f626

@smarter smarter requested a review from odersky October 1, 2017 15:34
@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

@liufengyun Are the tests still running?

@liufengyun
Copy link
Contributor

I just checked, there's now some issue with publishing website. I manually published below:

http://dotty-bench.epfl.ch/3222-1/index.html

But it only tests the first commit.

@liufengyun
Copy link
Contributor

test performance please

@dottybot
Copy link
Member

dottybot commented Oct 1, 2017

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

@smarter
Copy link
Member Author

smarter commented Oct 1, 2017

Hmm this is going to be misleading because more stuff has been merged between the last test command 4 hours ago and the new test command now.

@liufengyun
Copy link
Contributor

After the current test finishes, it's possible to run retest performance please to test against the latest master in a new test session.

@dottybot
Copy link
Member

dottybot commented Oct 1, 2017

Performance test finished successfully:

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

Benchmarks is based on merging with master (b7be521)

`classInfo(prevCtx)` does not reload the denotation at `prevCtx`, so the
if condition always returned true.
@smarter smarter force-pushed the fix-ensureFreshScopeAfter branch from ea1400f to 341bbd7 Compare October 1, 2017 20:00
@smarter smarter merged commit 299bb09 into scala:master Oct 1, 2017
@allanrenucci allanrenucci deleted the fix-ensureFreshScopeAfter branch December 14, 2017 19:20
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