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

Remove async playground doits #735

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

guillep
Copy link
Contributor

@guillep guillep commented Apr 4, 2024

As they can cause lots of unexpected concurrency issues.
They are a nice idea but they need more work.

See for example pharo-project/pharo#16311

More importantly, this breaks profiling completely, and has unexpected results that even experienced users will have difficulties reasoning about.

Here is an example of the potential performance issues we could have with a small example.
The async do it executes in low priority, which contends against the UI thread, giving overheads of at least ~4x in the following silly example.
And we have observed last week even worse cases and deadlocks.

So leaving a button that can deadlock your image is not very nice unless it has a dead head :)

Normal

[1000 timesRepeat: [ 
		1700 factorial.
		Transcript show: 'x'; cr ]
] timeToRun.

[AndreasSystemProfiler spyOn: [ 
	1000 timesRepeat: [ 
		1700 factorial.
		Transcript show: 'x'; cr ]
]] timeToRun.

"Async Do-it"		1.338s.
"Async+profiler"	1.818s.
"Sync Do-it"			0.569s.
"Sync+profiler"		0.706s.

High

[ t := [1000 timesRepeat: [ 
		1700 factorial.
		Transcript show: 'x'; cr ]
] timeToRun.] forkAt: 41.

[ t := [AndreasSystemProfiler spyOn: [ 
	1000 timesRepeat: [ 
		1700 factorial.
		Transcript show: 'x'; cr ]
]] timeToRun ] forkAt: 41.

"Async Do-it"		0.568s.
"Async+profiler"	0.708s.
"Sync Do-it"			0.582s.
"Sync+profiler"		0.713s.

@jecisc jecisc merged commit 47cb735 into pharo-spec:Pharo12 Apr 4, 2024
1 of 2 checks passed
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