DWR

I've got a fix for this issue, but I will need to create some automated tests before I submit the patch.

I've attached the patch file for the load montior and a class that semi tests it. This type of monitor is tough to test because it is based on the number of hits per second and without causing long delays in the junit tests it's almost impossible.

This will need some good peer review since I made some changes to the way the monitor works. At the basic level the monitor now runs a thread that will limit the amount of mode changes that occur which will help minimize thrashing under high loads.

The newest patch has a debug line that I forgot to remove.

Matt, I will start looking at it.

It would also be nice if we could add documentation on the load monitors. We have a docdwr project that you can checkout when you have time and add some docs describing this feature (how it works, what the default monitor is, how to configure the throttling monitor, etc.).

I'll take a look at the docs. Do we have a separate branch for the version 3 docs?

Trunk. Earlier versions are branched.

Ok, give the code a quick look when you get a chance and let me know if you think the concept is going to work. If so I will document it and we'll get it pushed out to the early adopters.

My initial thinking is that it looks good - but I want to give Mike a bit to look at it too.

I'll go ahead and document the code if you think it is on scope with the project. Docs can always be modified. Thanks!

Feel free to include this doc page in the reverse ajax section. It definitely isn't perfect, but it will act as a good place holder for now.

Matt the docs look very good. One minor typo:

"Note that the thread will verify that load at least once per hour even if no connections are being received."

Not sure exactly what verbage you wanted to use here, maybe just leave "that" out before load?

Good catch. I think removing "that" would be great. Thanks!

– Posted from Bugbox for Android

I have checked the load-monitors.html docs page into dwrdocs.

I seem to have missed looking at this but here's my late comments on the patch:

1) waitingThreads isn't synchronized everywhere which may lead to stale values being used. For maximum concurrency it might also be better to use an AtomicInteger instead of lock + sync block. This also solves the "stale" issue.

2) I think the class has effectively become a singleton now, with the static Thread design (only the first instance will be monitored by the thread). We should allow multiple instances of load monitors in the same class loader.

Based on these first two comments you may want to do some changes so I'll have a look again once that's done.
Best regards
Mike

Thanks for the review Mike!

1. Good catch.

• In most cases the cached value in the cpu would probably lead to an accurate enough result, but a volatile var may be better.

2. Another good catch.

• I'm guessing from your comment that lighting up more than one DWRServlet could also start another load monitor and in the static thread model cause both Servlets to get throttled the same way. My bad on this one, hadn't really thought about multiple DWR handlers.

I will make some changes and submit the fix.

• Matt -

Personally I would prefer AtomicInteger instead of volatile, to be able to get rid of the lock + sync blocks.

I agree with Mike. volatile will provide visibility but not atomicity in this case as we are not guaranteed the value will be written to from a single thread,

Have to bring out a quote from "Java Concurrency In Practice" here:

"You can use volatile variables only when all the following criteria are met:

• Writes to the variable do not depend on its current value, or you can ensure that only a single thread ever updates the value.
• The variable does not participate in invariants with other state variables; and
• Locking is not required for any other reason while the variable is being accessed"

I was going to run the volatile inside of a sync block so that the atomic piece was handled, but after some research... never again. It is slightly faster to run a volatile inside of a sync block and then read the volatile var without syncing than synchronizing every read and write, but since the concurrent.atomic package does some low level JVM work it is twice as fast as that scenario. Thanks for the eye opener!

I will make the necessary changes and send them out.

Mike, is #2 above correct? Does the load monitor get started for each instance of the servlet?

• Matt -

Yeah, I try to use the atomic classes whenever I can, partly because they are a "better volatile" in both functionality and performance, and partly because they don't lock threads (no sync blocks) so you by definition can't end up in deadlocks. (Get a copy of the JCIP book if you are interested in this kind of stuff)

Wrt #2, yes each servlet instance sets up its own Container instance with its own "other" instances.

JCIP. Check.
Atomic classes must have been new in Java 5. Kinda cool. Thanks!

Yes, new in Java 5 (some additions in Java 6), and that's why we couldn't use them in DWR2 that's compatible with Java 1.3, while DWR3 is based off Java 5.

Attached are the updates to use Atomic based variables and to allow a thread per instance monitoring solution. Let me know if this is more of what you were looking for.

• Matt -

I don't know how safe this change is, but I am using the java.*.atomic package hammer.

Great, Matt. I hope to have time to look at it next week.

Sorry, didn't get to this before my recent travelling, will look at it ASAP!

I'm still looking at the patch but would like to ask one thing: is the ThrottlingServerLoadMonitor Thread important or could we as well call the loadAdjust method from selected request threads? (say every 100th request or every 10 secs or something)

The idea behind having the thread run periodically is that it won't penalize any single user, but instead will penalize all users equally dependent on the server resources. In this case it may be a little extreme given the amount of time a load adjustment takes, but the user that triggers the load adjustment would be penalized.

Ok, then I'll regard it as possible to get rid of the thread if we want. I'll think a little about it.
Why I'm mentioning this is because we should try to avoid unnecessary thread handling as some environments that we want to be compatible with in the future don't allow us to create threads, such as Google App Engine.

Hi Matt, sorry for the extended delay . I've done my review now and it looks good apart from one thing I'd like you to find an alternative for. You have two different lock objects in your load adjustment thread, could you alter the code so you are only using one? This is really "defensive" programming, but it's quite easy to end up in deadlock scenarios, forgetting to notify all locks, etc, when using several locks.
Once that is fixed I expect you can commit your changes to trunk!

My apologies for the extremely extended delay. Attached is an updated monitor that I believe fits better into the vision of the monitor. Let me know what you think.