Rename EWYK The AdaptiveExecutionStrategy #6353

gregw · 2021-06-03T11:42:42Z

Rename EWYK to AdaptiveExecutionStrategy, which better represents the nature of the strategy.
#6391
Signed-off-by: Greg Wilkins gregw@webtide.com

Rename EWYK The AdaptiveExecutionStrategy, which better represents the nature of the strategy. Signed-off-by: Greg Wilkins <gregw@webtide.com>

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/AdaptiveExecutionStrategy.java

Updated the documentation from review, but in so doing realised that the sub-strategy selection could be more simply represented in code, so that was also updated. Signed-off-by: Greg Wilkins <gregw@webtide.com>

Added notes about chaining strategies and thread starvation Signed-off-by: Greg Wilkins <gregw@webtide.com>

Fixed formatting and made a little clearer Signed-off-by: Greg Wilkins <gregw@webtide.com>

Added deprecated version of EWYK Signed-off-by: Greg Wilkins <gregw@webtide.com>

lachlan-roberts

LGTM

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/AdaptiveExecutionStrategy.java

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/EatWhatYouKill.java

Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

jetty-util/src/main/java/org/eclipse/jetty/util/thread/AutoLock.java

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/AdaptiveExecutionStrategy.java

Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

version without code duplication Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw · 2021-06-09T22:01:51Z

@sbordet I have pushed a version without the lock hack that well matches the documented algorithm. Note also with this forumulation there is one less test the common case for a non-blocking task, so it will take a much simpler path.

The cost is that for the case of an potentially blocking task when there are no pending producers available, then the taskType is checked twice. This double check can be removed, but only by breaking out the EITHER and BLOCKING cases and thus duplicating the synchronized block and changing the flow from the documentation... but that may also be OK.... I'll add it as a comment to compare.

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/AdaptiveExecutionStrategy.java

Even more comments Some duplicated code to avoid double test on taskType Signed-off-by: Greg Wilkins <gregw@webtide.com>

hide the Runnable interface from the strategy signature Signed-off-by: Greg Wilkins <gregw@webtide.com>

fix accidental change Signed-off-by: Greg Wilkins <gregw@webtide.com>

refactor production and consumption methods to be smaller a better named/defined. Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw · 2021-06-10T03:47:15Z

@sbordet So I have re-re-re-reworked this class so:

The documentation is in the format you asked for
The actual code now actually matches the algorithm as described in the documentation
It has lots of good comments everywhere, but hopefully none that don't add value
the decomposition of the algorithm into methods has changed bit, giving smaller more meaningful methods (eg selectSubStrategy). Hopefully these will inlined to the same result anyway.
The only substantial change to the algorithm is that by first checking the taskType, then we can avoid a needless check on the thread type for PC tasks.

Considering the effort expended on a "simple" rename, please give this version fair consideration before just saying: NAH let's keep the old one

Note that I think there is some scope to manually unwind the production loop so that there is a blocking and a nonblocking version. I did give that a try, but code clarity suffered a lot and I think there would only be very marginal gains if any. The evidence is that the majority of productions are done with blocking invocations, so that path should be well optimized by the JIT without manual unwinding.

updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw · 2021-06-12T05:38:56Z

@sbordet nudge... let's get this merged long before next release train

sbordet

Looks good apart niggles in the comment/javadocs.

Since many javadocs were added, a general rule for javadocs is that they should be written in 3rd person, therefore "Selects a sub-strategy" rather than "Select a sub-strategy", "Runs" rather than "Run", etc.
See:
https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#styleguide
https://blog.joda.org/2012/11/javadoc-coding-standards.html

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/AdaptiveExecutionStrategy.java

niggles for javadoc Signed-off-by: Greg Wilkins <gregw@webtide.com>

sbordet

Not all javadocs were put in 3rd person, it's just to have a little more consistency, but I don't want to annoy too much. LGTM.

more niggles for javadoc Signed-off-by: Greg Wilkins <gregw@webtide.com>

Fixed javadoc 3rd person forms. Removed stale references to doProduce() method. Using IO.close(Closeable). Other small cosmetic changes. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw · 2021-06-15T23:06:56Z

@sbordet nudge

sbordet · 2021-06-15T23:07:59Z

@gregw I made further changes to the javadocs, see my commit.

gregw · 2021-06-16T07:15:51Z

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/AdaptiveExecutionStrategy.java

     * @return True if the sub-strategy requires the caller to continue to produce tasks.
     */
    private boolean consumeTask(Runnable task, SubStrategy subStrategy)
    {
        // Consume and/or execute task according to the selected mode.
        if (LOG.isDebugEnabled())
-            LOG.debug("{} ss={} t={}/{} {}", this, subStrategy, task, Invocable.getInvocationType(task));
+            LOG.debug("ss={} t={}/{} {}", subStrategy, task, Invocable.getInvocationType(task), this);


@sbordet this is now inconsistent with the other debugs in the class.... but I guess that is not so important.

gregw requested a review from sbordet June 3, 2021 11:43

Rename EWYK The AdaptiveExecutionStrategy

f479c4d

Rename EWYK The AdaptiveExecutionStrategy, which better represents the nature of the strategy. Signed-off-by: Greg Wilkins <gregw@webtide.com>

sbordet requested changes Jun 3, 2021

View reviewed changes

gregw added 8 commits June 4, 2021 09:18

Rename EWYK The AdaptiveExecutionStrategy

1d7b614

Updated the documentation from review, but in so doing realised that the sub-strategy selection could be more simply represented in code, so that was also updated. Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

28bc933

Updated the documentation from review, but in so doing realised that the sub-strategy selection could be more simply represented in code, so that was also updated. Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

cc0efd2

Updated the documentation from review, but in so doing realised that the sub-strategy selection could be more simply represented in code, so that was also updated. Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

194e638

Added notes about chaining strategies and thread starvation Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

21e70b8

Fixed formatting and made a little clearer Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

325719c

Added deprecated version of EWYK Signed-off-by: Greg Wilkins <gregw@webtide.com>

Update from review

e2bbcf0

Fixed format

68a7391

gregw requested review from lachlan-roberts and sbordet June 4, 2021 06:03

lachlan-roberts previously approved these changes Jun 7, 2021

View reviewed changes

sbordet requested changes Jun 7, 2021

View reviewed changes

Rename EWYK The AdaptiveExecutionStrategy

9374220

Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw dismissed lachlan-roberts’s stale review via 9374220 June 8, 2021 02:31

gregw requested review from lachlan-roberts and sbordet June 8, 2021 23:20

sbordet requested changes Jun 9, 2021

View reviewed changes

jetty-util/src/main/java/org/eclipse/jetty/util/thread/AutoLock.java Outdated Show resolved Hide resolved

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/AdaptiveExecutionStrategy.java Outdated Show resolved Hide resolved

gregw added 2 commits June 10, 2021 07:41

Rename EWYK The AdaptiveExecutionStrategy

0593f13

Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

59dab00

version without code duplication Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw commented Jun 9, 2021

View reviewed changes

jetty-util/src/main/java/org/eclipse/jetty/util/thread/strategy/AdaptiveExecutionStrategy.java Outdated Show resolved Hide resolved

gregw requested a review from sbordet June 9, 2021 22:06

gregw added 4 commits June 10, 2021 11:13

Rename EWYK The AdaptiveExecutionStrategy

feca6eb

Even more comments Some duplicated code to avoid double test on taskType Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

395173e

hide the Runnable interface from the strategy signature Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

c0a358d

fix accidental change Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

c2ecdc4

refactor production and consumption methods to be smaller a better named/defined. Signed-off-by: Greg Wilkins <gregw@webtide.com>

Rename EWYK The AdaptiveExecutionStrategy

8dd8f1b

updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

lachlan-roberts previously approved these changes Jun 10, 2021

View reviewed changes

gregw linked an issue Jun 10, 2021 that may be closed by this pull request

Rename EatWhatYouKill to AdaptiveExecutionStrategy #6391

Closed

sbordet previously approved these changes Jun 14, 2021

View reviewed changes

Rename EWYK The AdaptiveExecutionStrategy

8acfd44

niggles for javadoc Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw dismissed stale reviews from sbordet and lachlan-roberts via 8acfd44 June 14, 2021 23:17

gregw requested review from sbordet and lachlan-roberts June 15, 2021 01:42

lachlan-roberts previously approved these changes Jun 15, 2021

View reviewed changes

sbordet previously approved these changes Jun 15, 2021

View reviewed changes

Rename EWYK The AdaptiveExecutionStrategy

d2dee7b

more niggles for javadoc Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw dismissed stale reviews from sbordet and lachlan-roberts via d2dee7b June 15, 2021 12:36

sbordet self-requested a review June 15, 2021 14:22

Fixes #6391 - Rename EatWhatYouKill to AdaptiveExecutionStrategy.

f79ec97

Fixed javadoc 3rd person forms. Removed stale references to doProduce() method. Using IO.close(Closeable). Other small cosmetic changes. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet approved these changes Jun 15, 2021

View reviewed changes

gregw commented Jun 16, 2021

View reviewed changes

gregw merged commit a415606 into jetty-10.0.x Jun 16, 2021

joakime deleted the jetty-10.0.x-AdaptiveExecutionStrategy branch June 17, 2021 12:45

Rename EWYK The AdaptiveExecutionStrategy #6353

Rename EWYK The AdaptiveExecutionStrategy #6353

Conversation

gregw commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lachlan-roberts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gregw commented Jun 9, 2021

Uh oh!

Uh oh!

gregw commented Jun 10, 2021

Uh oh!

gregw commented Jun 12, 2021

Uh oh!

sbordet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbordet left a comment

Choose a reason for hiding this comment

Uh oh!

gregw commented Jun 15, 2021

Uh oh!

sbordet commented Jun 15, 2021

Uh oh!

gregw Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

gregw commented Jun 3, 2021 •

edited

Loading