Prefer instanceOf instead of getClass() in AsciiString by doom369 · Pull Request #9366 · netty/netty

doom369 · 2019-07-14T09:08:48Z

Motivation:

instanceOf doesn't perform null check like getClass() does. So instanceOf may be faster. However, it not true for all cases, as C2 could eliminate these null checks for getClass().

Modification:

Replaced string.getClass() == AsciiString.class with string instanceof AsciiString.

Proof:

@BenchmarkMode(Mode.Throughput)
@Fork(value = 1)
@State(Scope.Thread)
@Warmup(iterations = 5, time = 1, batchSize = 1000)
@Measurement(iterations = 10, time = 1, batchSize = 1000)
public class GetClassInstanceOf {

    Object key;

    @Setup
    public void setup() {
        key = "123";
    }

    @Benchmark
    public boolean getClassEquals() {
        return key.getClass() == String.class;
    }

    @Benchmark
    public boolean instanceOf() {
        return key instanceof String;
    }

}

Benchmark                           Mode  Cnt       Score      Error  Units
GetClassInstanceOf.getClassEquals  thrpt   10  401863.130 ± 3092.568  ops/s
GetClassInstanceOf.instanceOf      thrpt   10  421386.176 ± 4317.328  ops/s

netty-bot · 2019-07-14T09:09:04Z

Can one of the admins verify this patch?

franz1981 · 2019-07-14T09:32:07Z

AFAIK https://gist.github.com/apangin/7a9b7062a4bd0cd41fcc getClass is already a JVM intrinsic: can you please check why it is supposed to be faster?
My bet is due to some null check and worth to check what happen if you call it with different subclasses (if any) of AsciiString.
In addition, beware that it would slightly change the semantic, given that getClass will consider an exact match while instance of include all the subclasses/implementors...

normanmaurer · 2019-07-14T09:38:56Z

@franz1981 as AsciiString is Final the semantics are the Same

franz1981 · 2019-07-14T09:46:12Z

@normanmaurer haven't considered it, you're right...that make me more curious to know why getClass is kinda slower...probably it is really due to null check..

doom369 · 2019-07-14T09:58:33Z

@franz1981

getClass() could be intrinsic. It doesn't marked with @HotSpotIntrinsicCandidate annotation. Not sure it is 100% guarantee.
Anyway, I don't think null check will give such big difference. I think main diff is in a number of byte code instructions:

    ALOAD 0
    GETFIELD cc/microbenchmarks/A.key : Ljava/lang/Object;
    INSTANCEOF java/lang/String
    IRETURN

    ALOAD 0
    GETFIELD cc/microbenchmarks/A.key : Ljava/lang/Object;
    INVOKEVIRTUAL java/lang/Object.getClass ()Ljava/lang/Class;
    LDC Ljava/lang/String;.class
    IF_ACMPNE L1
    ICONST_1
    GOTO L2
   L1
   FRAME SAME
    ICONST_0
   L2
   FRAME SAME1 I
    IRETURN

So it is probably not only null check but also class compare check.

franz1981 · 2019-07-14T12:58:19Z

Only ASM check could tell us the truth: as long as the methods got hot, I believe they will be replaced by their ASM replacements (4 the intrinsics) and the C2 compiled version for the rest of the code, so I won't be worried too much by the bytecode instructions...

doom369 · 2019-07-14T13:45:32Z

@franz1981 they will and they are. You can check that by warmup phase that is 30% slower. However, still instanceof is faster after C2. So it just do less, whatever it does :).

normanmaurer · 2019-07-14T19:22:56Z

@netty-bot test this please

njhill · 2019-07-15T16:51:44Z

@doom369 looks good generally but how come you used String rather than AsciiString in the benchmarks? I would suggest repeating the bench with following changes:

Change to test for AsciiString
Change declared key type to CharSequence
Include methods for both String and AsciiString-valued keys
Include methods which perform some kind of dereference before or after, e.g. return key.isEmpty() || key instanceof AsciiString ... since this exists in any "real" usage and I would assume the null check gets elided

doom369 · 2019-07-15T17:56:21Z

@BenchmarkMode(Mode.Throughput)
@Fork(value = 1)
@State(Scope.Thread)
@Warmup(iterations = 5, time = 1, batchSize = 1000)
@Measurement(iterations = 10, time = 1, batchSize = 1000)
public class GetClassInstanceOf {

    CharSequence keyAsciiString;
    CharSequence keyString;

    @Setup
    public void setup() {
        keyAsciiString = new AsciiString("123");
        keyString = "123";
    }

    @Benchmark
    public boolean getClassEqualsAsciiString() {
        return keyAsciiString.getClass() == AsciiString.class;
    }

    @Benchmark
    public boolean instanceOfAsciiString() {
        return keyAsciiString instanceof AsciiString;
    }

    @Benchmark
    public boolean getClassEquals() {
        return keyString.getClass() == AsciiString.class;
    }

    @Benchmark
    public boolean instanceOf() {
        return keyString instanceof AsciiString;
    }

}

Benchmark                                      Mode  Cnt       Score       Error  Units
GetClassInstanceOf.getClassEquals             thrpt   10  397689.117 ± 14245.477  ops/s
GetClassInstanceOf.getClassEqualsAsciiString  thrpt   10  382372.886 ±  1538.955  ops/s
GetClassInstanceOf.instanceOf                 thrpt   10  420783.356 ±  4398.629  ops/s
GetClassInstanceOf.instanceOfAsciiString      thrpt   10  412792.946 ±  1816.614  ops/s

@njhill no changes.

doom369 · 2019-07-15T18:02:58Z

@njhill @franz1981 look like this is null check after all.

@BenchmarkMode(Mode.Throughput)
@Fork(value = 1)
@State(Scope.Thread)
@Warmup(iterations = 5, time = 1, batchSize = 1000)
@Measurement(iterations = 10, time = 1, batchSize = 1000)
public class GetClassInstanceOf {

    CharSequence keyAsciiString;
    CharSequence keyString;

    @Setup
    public void setup() {
        keyAsciiString = new AsciiString("123");
        keyString = "123";
    }

    @Benchmark
    public boolean getClassEqualsAsciiStringDeref() {
        return keyAsciiString.length() > 0 && keyAsciiString.getClass() == AsciiString.class;
    }

    @Benchmark
    public boolean instanceOfAsciiStringDeref() {
        return keyAsciiString.length() > 0 && keyAsciiString instanceof AsciiString;
    }

}

GetClassInstanceOf.getClassEqualsAsciiStringDeref  thrpt   10  366152.402 ± 5248.232  ops/s
GetClassInstanceOf.instanceOfAsciiStringDeref      thrpt   10  369959.579 ± 4106.474  ops/s

normanmaurer · 2019-07-16T11:10:43Z

@njhill @franz1981 so what you think? Even if it not shows a perf win I kind of think we should pull it in just for the sake of consistency

franz1981 · 2019-07-16T12:32:31Z

@normanmaurer I will push this but will avoid on it to refer to any perf improvement to save next PRs to rely on the same (inaccurate) assumption

normanmaurer · 2019-07-16T12:53:02Z

@franz1981 sounds good

njhill

LGTM

normanmaurer · 2019-07-16T14:47:29Z

@doom369 can you adjust the commit message ?

doom369 · 2019-07-16T14:55:25Z

@normanmaurer you mean edit text in PR text?

normanmaurer · 2019-07-16T15:09:02Z

@doom369 yes please (and also the commit message by amend it).

doom369 · 2019-07-16T15:32:45Z

I fixed the PR message. Regarding commit message you can change it during PR merge. (I still don't understand what should I change in it anyway)

normanmaurer · 2019-07-16T19:20:20Z

@doom369 thanks a lot!

@measurement

Motivation: `instanceOf` doesn't perform null check like `getClass()` does. So `instanceOf` may be faster. However, it not true for all cases, as C2 could eliminate these null checks for `getClass()`. Modification: Replaced `string.getClass() == AsciiString.class` with `string instanceof AsciiString`. Proof: ``` @BenchmarkMode(Mode.Throughput) @fork(value = 1) @State(Scope.Thread) @WarmUp(iterations = 5, time = 1, batchSize = 1000) @measurement(iterations = 10, time = 1, batchSize = 1000) public class GetClassInstanceOf { Object key; @setup public void setup() { key = "123"; } @benchmark public boolean getClassEquals() { return key.getClass() == String.class; } @benchmark public boolean instanceOf() { return key instanceof String; } } ``` ``` Benchmark Mode Cnt Score Error Units GetClassInstanceOf.getClassEquals thrpt 10 401863.130 ± 3092.568 ops/s GetClassInstanceOf.instanceOf thrpt 10 421386.176 ± 4317.328 ops/s ```

prefer instanceOf instead of getClass()

1aace58

franz1981 approved these changes Jul 14, 2019

View reviewed changes

normanmaurer approved these changes Jul 14, 2019

View reviewed changes

normanmaurer requested a review from njhill July 15, 2019 13:33

njhill approved these changes Jul 16, 2019

View reviewed changes

normanmaurer merged commit a82d62a into netty:4.1 Jul 16, 2019

normanmaurer added this to the 4.1.38.Final milestone Jul 16, 2019

Uh oh!

Conversation

doom369 commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netty-bot commented Jul 14, 2019

Uh oh!

franz1981 commented Jul 14, 2019

Uh oh!

normanmaurer commented Jul 14, 2019

Uh oh!

franz1981 commented Jul 14, 2019

Uh oh!

doom369 commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

franz1981 commented Jul 14, 2019

Uh oh!

doom369 commented Jul 14, 2019

Uh oh!

normanmaurer commented Jul 14, 2019

Uh oh!

njhill commented Jul 15, 2019

Uh oh!

doom369 commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doom369 commented Jul 15, 2019

Uh oh!

normanmaurer commented Jul 16, 2019

Uh oh!

franz1981 commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Jul 16, 2019

Uh oh!

njhill left a comment

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Jul 16, 2019

Uh oh!

doom369 commented Jul 16, 2019

Uh oh!

normanmaurer commented Jul 16, 2019

Uh oh!

doom369 commented Jul 16, 2019

Uh oh!

normanmaurer commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

doom369 commented Jul 14, 2019 •

edited

Loading

doom369 commented Jul 14, 2019 •

edited

Loading

doom369 commented Jul 15, 2019 •

edited

Loading

franz1981 commented Jul 16, 2019 •

edited

Loading