Skip to content

Added Benchmark for Vector#126

Merged
smarr merged 7 commits into
SOM-st:masterfrom
rhys-h-walker:master
Jul 4, 2025
Merged

Added Benchmark for Vector#126
smarr merged 7 commits into
SOM-st:masterfrom
rhys-h-walker:master

Conversation

@rhys-h-walker

@rhys-h-walker rhys-h-walker commented May 20, 2025

Copy link
Copy Markdown

This PR adds two benchmarks:

  1. VectorBenchmark, a simple benchmark using Vector's #,, #remove:, and #at:put: operations
  2. Knapsack, which is implemented using vectors and creates various vectors and accesses them with #at:, #at:put:, #append:`.

The PR also adds fallback to the SOM-st repos on GitHub actions when neither the user nor the pull request target have a fork of the SOM implementation.

Finally, we added two tests to VectorTest:

  • testAsArrayEnsureItCopiesStorageArray
  • testDoIndexesDoubleRemove

Core-Lib Updates

Status SOM PR
SOM (java) SOM-st/som-java#39
TruffleSOM SOM-st/TruffleSOM#234
PySOM SOM-st/PySOM#66
JsSOM SOM-st/JsSOM#20
SOM++ SOM-st/SOMpp#71
SOM-RS https://github.com/Hirevo/som-rs/pull/

@smarr smarr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, though, I got a few thoughts still 😄

Comment thread Examples/Benchmarks/VectorBenchmark.som Outdated
Comment thread Examples/Benchmarks/VectorBenchmark.som Outdated
Comment thread Examples/Benchmarks/VectorBenchmark.som Outdated
@rhys-h-walker
rhys-h-walker requested a review from smarr May 20, 2025 12:38
Comment thread IntegrationTests/Framework/defs.py Outdated
Signed-off-by: Stefan Marr <git@stefan-marr.de>
@smarr

smarr commented Jun 27, 2025

Copy link
Copy Markdown
Member

@rhys-h-walker The new Knapsack benchmark fails on SOM++. It segfaults: https://git.cs.kent.ac.uk/sm951/SOMpp/-/jobs/89643#L561

It only segfaults with clang as compiler, COPYING GC, and -DUSE_TAGGING=false -DCACHE_INTEGER=false. Might you have a chance to run this in a debugger to see whether we can figure out what might be wrong on SOM++?

I was also wondering whether this here could be described with a loop instead of hardcoding it:
https://github.com/SOM-st/SOM/pull/126/files#diff-c82009848c590b352d12e85f3c145a445df825290ffd37a5b5f09682c73e4bf2R8-R207

@smarr

smarr commented Jun 27, 2025

Copy link
Copy Markdown
Member

Hi @josrepo we are in the process of integrating the benchmark you added. Do you happen to remember where you got that one from? Would be great, if we could name the original source of it. Thanks!

@josrepo

josrepo commented Jun 27, 2025

Copy link
Copy Markdown

Hi @smarr,

Yes I remember! It's from this Baeldung article:
https://www.baeldung.com/java-knapsack
See solution 5 "dynamic programming solution"

I'll paste it here for completeness:

int knapsackDP(int[] w, int[] v, int n, int W) {
    if (n <= 0 || W <= 0) {
        return 0;
    }

    int[][] m = new int[n + 1][W + 1];
    for (int j = 0; j <= W; j++) {
        m[0][j] = 0;
    }

    for (int i = 1; i <= n; i++) {
        for (int j = 1; j <= W; j++) { 
            if (w[i - 1] > j) {
                m[i][j] = m[i - 1][j];
            } else {
                m[i][j] = Math.max(
                  m[i - 1][j], 
                  m[i - 1][j - w[i - 1]] + v[i - 1]);
            }
        }
    }
    return m[n][W];
}

Best,
Jacob

@smarr

smarr commented Jun 27, 2025

Copy link
Copy Markdown
Member

@josrepo oh, thanks a lot for the fast reply! Do you also happen to remember where you got the item data from? Or whether that's somewhat random. Thanks!

@josrepo

josrepo commented Jun 27, 2025

Copy link
Copy Markdown

@smarr I'm pretty sure I just made it up! and ran it through the original Java version to determine what the result should be

@smarr

smarr commented Jun 27, 2025

Copy link
Copy Markdown
Member

Perfect. Thanks!

@smarr smarr added the enhancement Improves the implementation with something noteworthy label Jul 3, 2025
This works mostly. It also allows us to size the benchmark now, which is useful.
I would expect this is still maintaining the focus on Vector operations of the benchmark, but have not checked that to be sure.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
@smarr

smarr commented Jul 3, 2025

Copy link
Copy Markdown
Member

@rhys-h-walker I forced pushed to this branch my cleanups. Could you please have a look at my version of Knapsack, whether you see anything obviously problematic?

Thanks!

@rhys-h-walker

Copy link
Copy Markdown
Author

@rhys-h-walker I forced pushed to this branch my cleanups. Could you please have a look at my version of Knapsack, whether you see anything obviously problematic?

Thanks!

Looks good to me, quite a bit tidier than my solution :)

@smarr
smarr merged commit 7bf0413 into SOM-st:master Jul 4, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improves the implementation with something noteworthy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants