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

Move updateErrorPoints background work to UI thread #4325

Merged
merged 2 commits into from Mar 6, 2016

Conversation

Projects
None yet
3 participants
@qiubit
Contributor

qiubit commented Feb 23, 2016

This pull request addresses #4322 and provides alternative solution of this problem than #4324, as suggested by @JakubValtar.

Instead of using Java synchronization mechanisms, which would sometimes cause UI thread to block until background thread in updateErrorPoints finishes execution, the work from background thread is now entirely done on the UI thread, which entirely eliminates synchronization issues (that's why all synchronized blocks were removed from updateErrorPoints).

Also, errorPoints redeclaration is fixed in order to be consistent with the initial declaration (which allows future errorPoints references from different threads to be safe - although maybe using synchronizedList now is unnecessary, given that all errorPoints references are from UI thread now?)

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Feb 24, 2016

Contributor

Thanks, great job!

If you still want to change it, I think we don't need synchronized list, regular ArrayList will do (it's accessed only from one thread). Also it should be enough to just clear the list instead of creating a new one every time update is called. Otherwise I think we are ready to merge it.

Contributor

JakubValtar commented Feb 24, 2016

Thanks, great job!

If you still want to change it, I think we don't need synchronized list, regular ArrayList will do (it's accessed only from one thread). Also it should be enough to just clear the list instead of creating a new one every time update is called. Otherwise I think we are ready to merge it.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Feb 25, 2016

Contributor

@benfry Can you please merge this? Fixes #4322, which is quite nasty.

Contributor

JakubValtar commented Feb 25, 2016

@benfry Can you please merge this? Fixes #4322, which is quite nasty.

benfry added a commit that referenced this pull request Mar 6, 2016

Merge pull request #4325 from qiubit/bugfix-errorPoints-v2
Move updateErrorPoints background work to UI thread

@benfry benfry merged commit 7cb020f into processing:master Mar 6, 2016

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Mar 6, 2016

Contributor

Thank you @qiubit for fixing this, it's now merged and will be part of the next release.

Contributor

JakubValtar commented Mar 6, 2016

Thank you @qiubit for fixing this, it's now merged and will be part of the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment