Skip to content

Entry into the contest, calculate_average_mtopolnik.sh - #246

Merged
gunnarmorling merged 61 commits into
gunnarmorling:mainfrom
mtopolnik:main
Jan 11, 2024
Merged

gunnarmorling merged 61 commits into
gunnarmorling:mainfrom
mtopolnik:main

Conversation

@mtopolnik

@mtopolnik mtopolnik commented Jan 8, 2024 •

Copy link
Copy Markdown
Contributor

Check List:

  • Tests pass (./test.sh <username> shows no differences between expected and actual outputs)
  • All formatting changes by the build are committed
  • Your launch script is named calculate_average_<username>.sh (make sure to match casing of your GH user name) and is executable
  • Output matches that of calculate_average_baseline.sh
  • Execution time: 5.6 seconds
  • Execution time of reference implementation: 250 seconds

I'm benchmarking on a Hetzner CCX3 in Helsinki, it uses the older Milan CPU generation (no AVX-512, i.e., matching the evaluation environment).

My primary focus was performance on the "new" dataset with 10,000 keys and names in the full range of 1-100 bytes. I find that the timing on that dataset degrades to 10.1 seconds.

You can generate the dataset with create_measurements3.sh, and I encourage contestants to try it out and use as the optimization target. It's unquestionably a kick to see oneself at the top of the leaderboard, but it's more fun (and useful!) to design a solution that works well beyond the 416 station names of max length 26 chars.

Here's an interesting way to compare the "old" and the "new" datasets: perf statistics.

perf stats for the "old" dataset

    10,526,045,348      branches                  #  241.214 M/sec                    (74.97%)
        51,816,057      branch-misses             #    0.49% of all branches          (75.01%)
     1,300,656,793      cache-references          #   29.806 M/sec                    (75.07%)
        67,109,208      cache-misses              #    5.160 % of all cache refs      (75.10%)
    66,603,846,862      cycles                    #    1.526 GHz                      (75.13%)
   153,953,175,967      instructions              #    2.31  insn per cycle         
                                                  #    0.00  stalled cycles per insn  (75.13%)
       653,594,159      idle-cycles-backend       #    0.98% backend cycles idle      (74.88%)
       120,516,668      idle-cycles-frontend      #    0.18% frontend cycles idle     (74.80%)
         43,637.86 msec task-clock                #    7.451 CPUs utilized          

       5.857020491 seconds time elapsed

      42.039450000 seconds user
       1.565338000 seconds sys
perf stats for the "new" dataset

    23,101,933,451      branches                  #  286.688 M/sec                    (75.02%)
       547,878,407      branch-misses             #    2.37% of all branches          (75.05%)
     3,088,422,339      cache-references          #   38.326 M/sec                    (75.09%)
       836,389,639      cache-misses              #   27.081 % of all cache refs      (75.13%)
   123,039,762,409      cycles                    #    1.527 GHz                      (75.12%)
   216,498,165,259      instructions              #    1.76  insn per cycle         
                                                  #    0.01  stalled cycles per insn  (75.03%)
     1,168,756,378      idle-cycles-backend       #    0.95% backend cycles idle      (74.82%)
       175,164,325      idle-cycles-frontend      #    0.14% frontend cycles idle     (74.82%)
         80,582.13 msec task-clock                #    7.436 CPUs utilized          

      10.837166912 seconds time elapsed

      77.902857000 seconds user
       2.619319000 seconds sys

One thing pops out: the "old" dataset gets away with just 0.06 cache misses and 0.05 branch misses per row of input! All the live slots of the hashtable fit into the L1 cache. We can see a notable impact on instructions per cycle: 2.31 on the "old" set, 1.76 on the "new" one.

@mtopolnik
mtopolnik marked this pull request as draft January 9, 2024 06:34
@mtopolnik
mtopolnik force-pushed the main branch 5 times, most recently from 248e5b0 to 7fb3e9b Compare January 9, 2024 10:28
@mtopolnik
mtopolnik marked this pull request as ready for review January 9, 2024 10:30
@gunnarmorling

Copy link
Copy Markdown
Owner

Hey @mtopolnik, I see you still push commits here. Should I hold off with running this for some more?

@mtopolnik

Copy link
Copy Markdown
Contributor Author

It's ready to merge, I just can't help fiddling some more while waiting :) I'll open another PR if I get anything worthwhile.

@gunnarmorling

Copy link
Copy Markdown
Owner

Ok, gotcha. Getting this when running tests:

Validating calculate_average_mtopolnik.sh -- src/test/resources/samples/measurements-10000-unique-keys.txt
Exception in thread "Thread-29" java.lang.OutOfMemoryError: Cannot reserve 16 bytes of direct buffer memory (allocated: 268437472, limit: 268435456)
	at java.base/java.nio.Bits.reserveMemory(Bits.java:178)
	at java.base/java.nio.Buffer$2.reserveMemory(Buffer.java:910)
	at java.base/jdk.internal.foreign.NativeMemorySegmentImpl.makeNativeSegment(NativeMemorySegmentImpl.java:127)
	at java.base/jdk.internal.foreign.MemorySessionImpl.allocate(MemorySessionImpl.java:158)
	at java.base/java.lang.foreign.Arena.allocate(Arena.java:273)
	at java.base/java.lang.foreign.SegmentAllocator.allocate(SegmentAllocator.java:346)
	at dev.morling.onebrc.CalculateAverage_mtopolnik$ChunkProcessor.run(CalculateAverage_mtopolnik.java:194)
	at java.base/java.lang.Thread.run(Thread.java:1583)
Exception in thread "main" java.lang.NullPointerException: Cannot read the array length because "results[i]" is null
	at dev.morling.onebrc.CalculateAverage_mtopolnik.calculate(CalculateAverage_mtopolnik.java:122)
	at dev.morling.onebrc.CalculateAverage_mtopolnik.main(CalculateAverage_mtopolnik.java:81)

@mtopolnik

Copy link
Copy Markdown
Contributor Author

Wow. Seems specific to that environment. I'll look into it.

@mtopolnik

Copy link
Copy Markdown
Contributor Author

Runtime.availableProcessors() returns 8 as before?

@gunnarmorling

Copy link
Copy Markdown
Owner

Runtime.availableProcessors() returns 8 as before?

Yepp, it does.

@mtopolnik

Copy link
Copy Markdown
Contributor Author

And just to make sure, how much GB RAM is available?

@gunnarmorling

gunnarmorling commented Jan 11, 2024 via email

Copy link
Copy Markdown
Owner

@mtopolnik

Copy link
Copy Markdown
Contributor Author

For one reason or another, it hits the Xmx=256m limit in the new environment. I pushed a change that removes Xmx.

@mtopolnik

Copy link
Copy Markdown
Contributor Author

I just noticed the error message states Thread-29 as the name of the thread. This is a pretty strong hint that availableProcessors() returned more than 8.

@gunnarmorling

Copy link
Copy Markdown
Owner

Hum, hum, interesting. Will look into it later today.

@gunnarmorling

Copy link
Copy Markdown
Owner

Very interesting. I can't seem to reproduce this (also when adding back the Xmx256m). I printed the result of Runtime.availableProcessors() from your program when run in the evaluation loop, and it shows 8, as expected. But then, I also took a thread dump and don't see those suspiciously high thread names either (only up to "Thread-7"). It's a bit mysterious tbh.

@gunnarmorling

Copy link
Copy Markdown
Owner

All looking good now. Kudos for pushing hard to support the larger key set, too! As discussed elsewhere, it would have been nice to do the same for the official challenge, but I don't think it should be changed now. Definitely a good learning for myself for the next time, I really appreciate it. We might also consider running the Top 10 or so on the full key set in the last week of the challenge or so (also want to run them on all the 64 cores of the testing machine).

You come in at 00:03.539, really good! Congrats and thanks for participating!

@gunnarmorling
gunnarmorling merged commit 95459f5 into gunnarmorling:main Jan 11, 2024
dmitry-midokura pushed a commit to dmitry-midokura/1brc that referenced this pull request Jan 13, 2024
…#246)

* calculate_average_mtopolnik

* short hash (just first 8 bytes of name)

* Remove unneeded checks

* Remove archiving classes

* 2x larger hashtable

* Add "set" to setters

* Simplify parsing temperature, remove newline search

* Reduce the size of the name slot

* Store name length and use to detect collision

* Reduce memory loads in parseTemperature

* Use short for min/max

* Extract constant for semicolon

* Fix script header

* Explicit bash shell in shebang

* Inline usage of broadcast semicolon

* Try vectorization

* Remove vectorization

* Go Unsafe

* Use SWAR temperature parsing by merykitty

* Inline some things

* Remove commented-out MemorySegment usage

* Inline namesMem.asSlice() invocation

* Try out JVM JIT flags

* Implement strcmp

* Remove unused instance variables

* Optimize hashing

* Put station name into hashtable

* Reorder method

* Remove usage of MemorySegment.getUtf8String

Replace with UNSAFE.copyMemory() and new String()

* Fix hashing bug

* Remove outdated comments

* Fix informative constants

* Use broadcastByte() more

* Improve method naming

* More hashing

* Revert more hashing

* Add commented-out code to hash 16 bytes

* Slight cleanup

* Align hashtable at cacheline boundary

* Add Graal Native image

* Revert Graal Native image

This reverts commit d916a42.

* Simplify shell script (no SDK selection)

* Move a constant, zero out hashtable on start

* Better name comparison

* Add prepare_mtopolnik.sh

* Cleaner idiom in name comparison

* AND instead of MOD for hashtable indexing

* Improve word masking code

* Fix formatting

* Reduce memory loads

* Remove endianness checks

* Avoid hash == 0 problem

* Fix subtle bug

* MergeSort of parellel results

* Touch up perf

* Touch up perf

* Remove -Xmx256m

* Extract result printing method

* Print allocation details on OOME

* Single mmap

* Use global allocation arena
fozbstudios pushed a commit to fozbstudios/1brc that referenced this pull request Sep 12, 2025
…#246)

* calculate_average_mtopolnik

* short hash (just first 8 bytes of name)

* Remove unneeded checks

* Remove archiving classes

* 2x larger hashtable

* Add "set" to setters

* Simplify parsing temperature, remove newline search

* Reduce the size of the name slot

* Store name length and use to detect collision

* Reduce memory loads in parseTemperature

* Use short for min/max

* Extract constant for semicolon

* Fix script header

* Explicit bash shell in shebang

* Inline usage of broadcast semicolon

* Try vectorization

* Remove vectorization

* Go Unsafe

* Use SWAR temperature parsing by merykitty

* Inline some things

* Remove commented-out MemorySegment usage

* Inline namesMem.asSlice() invocation

* Try out JVM JIT flags

* Implement strcmp

* Remove unused instance variables

* Optimize hashing

* Put station name into hashtable

* Reorder method

* Remove usage of MemorySegment.getUtf8String

Replace with UNSAFE.copyMemory() and new String()

* Fix hashing bug

* Remove outdated comments

* Fix informative constants

* Use broadcastByte() more

* Improve method naming

* More hashing

* Revert more hashing

* Add commented-out code to hash 16 bytes

* Slight cleanup

* Align hashtable at cacheline boundary

* Add Graal Native image

* Revert Graal Native image

This reverts commit d916a42.

* Simplify shell script (no SDK selection)

* Move a constant, zero out hashtable on start

* Better name comparison

* Add prepare_mtopolnik.sh

* Cleaner idiom in name comparison

* AND instead of MOD for hashtable indexing

* Improve word masking code

* Fix formatting

* Reduce memory loads

* Remove endianness checks

* Avoid hash == 0 problem

* Fix subtle bug

* MergeSort of parellel results

* Touch up perf

* Touch up perf

* Remove -Xmx256m

* Extract result printing method

* Print allocation details on OOME

* Single mmap

* Use global allocation arena
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants