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

Rewrite the computer thread system #218

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@SquidDev
Contributor

SquidDev commented May 8, 2017

This makes a couple of significant changes to the original system, to reduce the number of threads created and allowing for multiple threads in the future. There are several notable changes from the original implementation:

  • A blocking queue is used for the main task queue queue. This removes the need for the "monitor" variable and allows for multiple threads polling this queue in the future.
  • The thread used to execute tasks is "cached" between tasks, significantly reducing the number of threads which need to be created. If a task needs to be stopped then the thread is then terminated and a new one constructed, though this rarely happens.

I'd really like someone with more multi-threading/concurrency knowledge to go over this and ensure it is sound. I'm not entirely sure if I need the semaphore class, or if .wait() and .notify() are sufficient. Nor am I sure if I've got my locks in entirely the correct place.

@dmarcuse

dmarcuse approved these changes May 8, 2017 edited

Looked through quickly and didn't see anything concerning, might want to wait for someone else with better experience with concurrency.

}
}
catch( InterruptedException ignored )
{

This comment has been minimized.

@JLLeitschuh

JLLeitschuh May 9, 2017

Contributor

Reset the interrupt flag??

Thread.currentThread().interrupt();

http://www.yegor256.com/2015/10/20/interrupted-exception.html

This comment has been minimized.

@SquidDev

SquidDev May 9, 2017

Contributor

Yeah, I saw your comments on one of the older issues about this. I meaning to do it and forgetting :).

catch( Throwable e )
{
System.out.println( "ComputerCraft: Error running thread." );
e.printStackTrace( System.out );

This comment has been minimized.

@JLLeitschuh

JLLeitschuh May 9, 2017

Contributor

What if the interrupted exception gets thrown?
Do you want to handle that differently?
What about something like an OutOfMemoryError?

This comment has been minimized.

@SquidDev

SquidDev May 9, 2017

Contributor

This part was just a direct copy of the original implementation :). I think it would probably be safe just to catch RuntimeException, though we may also want to catch OrphanedThread.

@JLLeitschuh

This comment has been minimized.

Contributor

JLLeitschuh commented May 9, 2017

Your whole starting/stopping infrastructure seems to be covered by guava's service helper methods.

https://github.com/google/guava/wiki/ServiceExplained

Perhaps consider using those instead?

}
catch( InterruptedException ignored )
{
}

This comment has been minimized.

@JLLeitschuh

JLLeitschuh May 9, 2017

Contributor

See my comment about the InterruptedException below.

This comment has been minimized.

@SquidDev

SquidDev May 10, 2017

Contributor

Is this really needed here? The thread is going to terminate at this point anyway it doesn't seem worth it. I might be wrong though.

synchronized (m_monitor)
void submit( ITask task )

This comment has been minimized.

@JLLeitschuh

JLLeitschuh May 9, 2017

Contributor

Can submit be called by multiple threads concurrently?

This comment has been minimized.

@SquidDev

SquidDev May 9, 2017

Contributor

No. Each TaskRunner will handle exactly one TaskExecutor.

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented May 10, 2017

Your whole starting/stopping infrastructure seems to be covered by guava's service helper methods.

It sort of is. However, using that does seem rather overkill for a very simple problem. I'd also rather avoid introducing a dependency on Guava, so emulators don't have to bundle it (though I accidentally did that in #159).

@SquidDev

This comment has been minimized.

Contributor

SquidDev commented May 10, 2017

One thing worth noting is that if the computer cannot be aborted (such as in #174 or in string pattern matching) then the computer will spin forever. From what I can tell, LuaThread.State.lua_resume (namely line 326) does not appear to respond to interruptions. At a guess (I haven't investigated too much), it's because LuaThread.State.run also locks on the monitor.

Also worth noting that the synchronized blocks on m_machine cannot be interrupted, causing problems when a new thread is created. It might be better to switch to a ReentrantLock and use lockInterruptibly.

@JLLeitschuh

This comment has been minimized.

Contributor

JLLeitschuh commented May 10, 2017

I'd also rather avoid introducing a dependency on Guava, so emulators don't have to bundle it (though I accidentally did that in #159).

Doesn't Minecraft/forge have a dependency on Guava already?

However, using that does seem rather overkill for a very simple problem

The reason that I suggest this is because multi threading is a complicated problem and I always try to use guava's threading libraries because I know it will be better tested than my own attempts.

SquidDev added some commits May 8, 2017

Rewrite the computer thread system
This makes a couple of significant changes to the original system, to
reduce the number of threads created and allow for multiple threads in
the future. There are several notable changes from the original
implementation:

 - A blocking queue is used for the main task queue queue. This removes
   the need for the "monitor" variable and allows for multiple threads
   polling this queue in the future.
 - The thread used to execute tasks is "cached" between tasks,
   significantly reducing the number of threads which need to be
   created. If a task needs to be stopped then the thread is then
   terminated and a new one constructed, though this rarely happens.

@SquidDev SquidDev referenced this pull request Jun 14, 2018

Open

Socket Support #550

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