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 [email protected]

Rename EWYK The AdaptiveExecutionStrategy, which better represents the nature of the strategy. Signed-off-by: Greg Wilkins <[email protected]>

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 <[email protected]>

Added notes about chaining strategies and thread starvation Signed-off-by: Greg Wilkins <[email protected]>

Fixed formatting and made a little clearer Signed-off-by: Greg Wilkins <[email protected]>

Added deprecated version of EWYK Signed-off-by: Greg Wilkins <[email protected]>

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 <[email protected]>

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 <[email protected]>

version without code duplication Signed-off-by: Greg Wilkins <[email protected]>

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 <[email protected]>

hide the Runnable interface from the strategy signature Signed-off-by: Greg Wilkins <[email protected]>

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 <[email protected]>

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 <[email protected]>

Fixed javadoc 3rd person forms. Removed stale references to doProduce() method. Using IO.close(Closeable). Other small cosmetic changes. Signed-off-by: Simone Bordet <[email protected]>

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.

Rename EWYK The AdaptiveExecutionStrategy

bab30b2

Rename EWYK The AdaptiveExecutionStrategy, which better represents the nature of the strategy. Signed-off-by: Greg Wilkins <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Rename EWYK The AdaptiveExecutionStrategy

194e638

Added notes about chaining strategies and thread starvation Signed-off-by: Greg Wilkins <[email protected]>

Rename EWYK The AdaptiveExecutionStrategy

21e70b8

Fixed formatting and made a little clearer Signed-off-by: Greg Wilkins <[email protected]>

Rename EWYK The AdaptiveExecutionStrategy

325719c

Added deprecated version of EWYK Signed-off-by: Greg Wilkins <[email protected]>

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 <[email protected]>

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 <[email protected]>

Rename EWYK The AdaptiveExecutionStrategy

59dab00

version without code duplication Signed-off-by: Greg Wilkins <[email protected]>

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 2 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 <[email protected]>

Rename EWYK The AdaptiveExecutionStrategy

395173e

hide the Runnable interface from the strategy signature Signed-off-by: Greg Wilkins <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename EWYK The AdaptiveExecutionStrategy #6353

Rename EWYK The AdaptiveExecutionStrategy #6353

gregw commented Jun 3, 2021 •

edited

Loading

lachlan-roberts left a comment

gregw commented Jun 9, 2021

gregw commented Jun 12, 2021

sbordet left a comment

sbordet left a comment

gregw commented Jun 15, 2021

sbordet commented Jun 15, 2021

gregw Jun 16, 2021

Rename EWYK The AdaptiveExecutionStrategy #6353

Rename EWYK The AdaptiveExecutionStrategy #6353

Conversation

gregw commented Jun 3, 2021 • edited Loading

lachlan-roberts left a comment

Choose a reason for hiding this comment

gregw commented Jun 9, 2021

gregw commented Jun 12, 2021

sbordet left a comment

Choose a reason for hiding this comment

sbordet left a comment

Choose a reason for hiding this comment

gregw commented Jun 15, 2021

sbordet commented Jun 15, 2021

gregw Jun 16, 2021

Choose a reason for hiding this comment

gregw commented Jun 3, 2021 •

edited

Loading