Avoid yield #26
Merged
Avoid yield #26
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Locke
commented
Jan 15, 2017
| // This is here to avoid leaving the engine in | ||
| // a state that is right before changing its | ||
| // state | ||
| boolean shouldRemoveFirstCommand = false; |
Contributor
Author
There was a problem hiding this comment.
Note: synchronized (engineMode) is no longer needed as isBusyLock is locked just before the call of processNextCommand
Locke
commented
Jan 15, 2017
| break; | ||
| } | ||
|
|
||
| if (shouldRemoveFirstCommand) { |
Contributor
Author
There was a problem hiding this comment.
Note: isBusyChange.signalAll() is not needed here as it is called right after the call of processNextCommand
avoid recompilation of all files every time, caused by known yet unfixed bugs. Also update to recent version See: - https://stackoverflow.com/questions/16963012/maven-compiler-recompile-all-files-instead-modified/19653164#19653164 - https://issues.apache.org/jira/browse/MCOMPILER-209 - https://issues.apache.org/jira/browse/MCOMPILER-205
Useful for external projects depending on test classes like the TestEngineDriver.
PluginClassLoader.loadCatalog(..., "plugins/") tries to load plugins from the test-rsc resp. from the test-jar as the folder name conflicts with the actual plugin folder.
Replace Thread yielding with a blocking wait to improve cpu usage. As engineBusy and engineMode are not final it is dangerous to synchronize on them, therefore a ReentrantLock is used. For the blocking wait a Condition is used that is signaled each time when the result of isBusy might change.
Contributor
Author
|
@araschke since you recently pushed some commits and wrote in another issue that you oversaw that notification I assume no one noticed my two pull requests. As this PR contains multiple commits I'm also happy to split this one into multiple independent PRs if it makes your review easier. |
Member
|
Dear André,
sorry that we oversaw your PR and thank you for the reminder. I was
quite busy that year with other stuff, but next year I want to continue
the work on CoreASM. Then I'll have a look at your suggestion.
Thank you very much for your valuable input!
Best regards,
Alexander
Am 15.12.2017 um 17:30 schrieb André Wolski:
… @araschke <https://github.com/araschke> since you recently pushed some
commits and wrote in another issue that you oversaw that notification I
assume no one noticed my two pull requests.
As this PR contains multiple commits I'm also happy to split this one
into multiple independent PRs if it makes your review easier.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/ACf6MNp7St_mVCPPQ5hsbPR4mKnlg8Vaks5tAp6xgaJpZM4Lj8zB>.
--
Alexander Raschke
Institut für Softwaretechnik und Programmiersprachen
Fakultät für Ingenieurwissenschaften, Informatik und Psychologie
Universität Ulm
89069 Ulm
Telefon: 0731/50-24172
Fax: 0731/50-12-24172
eMail: alexander.raschke@uni-ulm.de
Raum: O27 / 4105
|
…ueue I couldn't measure a perfomance difference, but this design looks better than something custom
Again: no measureable performance difference, but nicer design
This allows the Engine to be idle for a longer period of time without constantly yielding. - the updates of `engineBusy` and locks of `isBusyLock` should now be easier to follow -* i.e. idle == being in a blocking wait for the next command - `isBusyChanged` has been changed to `isNotBusy`
Locke
referenced
this pull request
in Locke/coreasm.core
Jan 10, 2019
The main loop assumes that the engine is busy, however the handling of the emError state sets engineBusy to false. This causes an assertion, that was added in c750929 via #26, to trigger, which leads in debug runs (-ea flag set) to a crashed Engine and a blocked TestEngineDriver. Additionaly the engineBusy flag is set to false even when the Engine is actually busy as the emError state is left to either emIdle or emTerminating. This commit 1) changes the assertion to not throw when in emError state and 2) corrects the setting of engineBusy to be false only when no ecTerminate / ecRecover command was handled. TODO: This basic fix leaves the Engine in a Thread.yield loop until a ecTerminate / ecRecover command is present. The polling of the commandQueue should be replaced with a blocking wait.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Occasionally some of my tests kept stuck in an endless loop. As it was not reproducible I didn't create an issue so far. However this week it got worse, so I decided to take a deeper look into it.
In order to narrow it down I rewrote waitWhileBusy in the Engine to use a blocking wait instead of
Thread.yieldwhich fortunately improved the execution time noticeably.The actual issue turned out to be a race condition in the TestEngineDriver. It happened sometimes that one thread was waiting in
resumefor the status to become running while the inner thread was kept in a shouldPause loop.Sketch of a possible execution order:
This only happened on a Core2 Duo with Linux, never on a i7 with 4 physical cores (8 logical) on Win10. As there are only two cores on the Linux machine I suspect the scheduler kept the yielded outer thread longer paused while it let the inner thread and the Engines threads running.
I began writing locks for the TestEngineDriver too, but noticed that the execution so tightly coupled that (in my eyes) it makes no sense at all to have an inner thread in the first place: executeSteps is blocked as long as the inner thread is running, and the inner thread is waiting for work until the next executeSteps is called. Therefore I removed the inner thread completely.
This PR also brings in some smaller changes to the pom.xml Feel free to pick only single commits, they should be completely independent.