Skip to content

Conversation

@rhys-h-walker
Copy link

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

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/

Copy link
Member

@smarr smarr left a comment

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 😄

@rhys-h-walker rhys-h-walker requested a review from smarr May 20, 2025 12:38
@smarr
Copy link
Member

smarr commented Jun 27, 2025

@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
Copy link
Member

smarr commented Jun 27, 2025

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
Copy link

josrepo commented Jun 27, 2025

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
Copy link
Member

smarr commented Jun 27, 2025

@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
Copy link

josrepo commented Jun 27, 2025

@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
Copy link
Member

smarr commented Jun 27, 2025

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 <[email protected]>
@smarr
Copy link
Member

smarr commented Jul 3, 2025

@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
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