WIP: Development benchmark DO NOT MERGE THIS MR IS FOR DEMONSTRATION PURPOSES ONLY #189

Closed jdonkervliet closed 4 years ago

In GitLab by @julian9499 on Jun 21, 2020, 15:12

In GitLab by @julian9499 on Jun 21, 2020, 15:14

added 1 commit

83a38a17 - fix checkstyle

In GitLab by @julian9499 on Jun 21, 2020, 15:24

marked as a Work In Progress

In GitLab by @julian9499 on Jun 21, 2020, 15:24

changed title from {-Development benchmark-} to {+WIP: Development benchmark DO NOT MERGE THIS MR IS FOR DEMONSTRATION PURPOSES ONLY+}

In GitLab by @swabbur on Jun 21, 2020, 15:25

Commented on src/main/java/net/glowstone/Benchmarker.java line 37

All chopped down lines should start with a +.

In GitLab by @swabbur on Jun 21, 2020, 15:26

Commented on src/main/java/net/glowstone/Benchmarker.java line 39

Why the limit on the number of elements in the queue? Wouldn't it make sense to allow as many as possible such that the main thread is never waiting on the thread writing the data?

In GitLab by @swabbur on Jun 21, 2020, 15:27

Commented on src/main/java/net/glowstone/Benchmarker.java line 39

Why is this a static member while the the remainder of the Benchmarker has been made non-static?

In GitLab by @swabbur on Jun 21, 2020, 15:28

Commented on src/main/java/net/glowstone/Benchmarker.java line 19

Missing whiteline.

In GitLab by @swabbur on Jun 21, 2020, 15:28

Commented on src/main/java/net/glowstone/Benchmarker.java line 20

missing whiteline.

In GitLab by @swabbur on Jun 21, 2020, 15:29

Commented on src/main/java/net/glowstone/Benchmarker.java line 47

Not all brokers have a channel.

In GitLab by @swabbur on Jun 21, 2020, 15:30

Commented on src/main/java/net/glowstone/Benchmarker.java line 50

Could you move the LocalDateTime to a separate value? Also, isn't there a DateFormatter specifically designed to make sure it's always in the same format? The default format changes based on region and system settings.

In GitLab by @swabbur on Jun 21, 2020, 15:31

Commented on src/main/java/net/glowstone/Benchmarker.java line 52

It might be neater to move the lambda to a separate method and reference it as this::run instead. Also, you could/should name the thread. That makes debugging easier.

In GitLab by @swabbur on Jun 21, 2020, 15:33

Commented on src/main/java/net/glowstone/Benchmarker.java line 68

Why divide by 50?

In GitLab by @swabbur on Jun 21, 2020, 15:33

Commented on src/main/java/net/glowstone/scheduler/GlowScheduler.java line 134

Might be neater to retrieve the broker configuration on a separate line.

In GitLab by @swabbur on Jun 21, 2020, 15:34

Commented on src/main/java/net/glowstone/Benchmarker.java line 45

Creating the Benchmarker directly from the config makes it harder to test.

In GitLab by @swabbur on Jun 21, 2020, 15:36

Commented on src/main/java/net/glowstone/scheduler/GlowScheduler.java line 232

You should NEVER measure time with currentTimeMillis. Always use nanoTime instead. currentTimeMillis is NOT monotonically increasing. And, nanoTime can be more precise, it is guaranteed to be at least as accurate as currentTimeMillis.

In GitLab by @swabbur on Jun 21, 2020, 15:37

Commented on src/main/java/net/glowstone/scheduler/GlowScheduler.java line 298

Why the reference to this?

In GitLab by @julian9499 on Jun 21, 2020, 15:39

Commented on src/main/java/net/glowstone/Benchmarker.java line 68

Due too the tick rate being 50, but I can change /50.00) * 100.0 to *2.0 if you want. It should be done by the compiler already

In GitLab by @julian9499 on Jun 21, 2020, 15:39

Commented on src/main/java/net/glowstone/Benchmarker.java line 52

Oke

In GitLab by @swabbur on Jun 21, 2020, 15:39

Commented on src/main/java/net/glowstone/Benchmarker.java line 37

Also, it might be neater to perform the formatting in the actual writer. That way, the BenchmarkData class doesn't need to be changed if the output format changes.

In GitLab by @swabbur on Jun 21, 2020, 15:43

Commented on src/main/java/net/glowstone/scheduler/GlowScheduler.java line 298

Should we measure the relative utilization of world only, instead of the utilization of everything?

In GitLab by @JimVliet on Jun 21, 2020, 15:45

Commented on src/main/java/net/glowstone/scheduler/GlowScheduler.java line 298

We measure the relative utilization of ticks, not just the world.

In GitLab by @swabbur on Jun 21, 2020, 15:47

Commented on src/main/java/net/glowstone/Benchmarker.java line 53

Missing column names.

In GitLab by @swabbur on Jun 21, 2020, 15:52

Commented on src/main/java/net/glowstone/scheduler/GlowScheduler.java line 298

But each world has a separate tick.