support GenericMemory by making the storage type a parameter

[nsajko]

This approach isolates us from possible changes in the GenericMemory design while leaving open the possibility of supporting other underlying storage types in the future.

Fixes #33

[nsajko]

@oscardssmith what do you think about the design? It's supposed to minimally restrict future developments/changes to GenericMemory.

[nsajko]

The tests are failing because of JuliaLang/julia#49978. Is it possible to fix this on the CI side, perhaps by running the tests without generating coverage data, and then doing an additional run for coverage?

[oscardssmith]

The overall design looks pretty reasonable.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.64%. Comparing base (03117ab) to head (652f1de).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   98.26%   98.64%   +0.37%     
==========================================
  Files           1        1              
  Lines         173      221      +48     
==========================================
+ Hits          170      218      +48     
  Misses          3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nsajko]

The tests should pass now, as the no-alloc tests are now conditional on the value of an environment variable.

Ideally we'd add new CI runs which would have the environment variable set to true while disabling code coverage.

[giordano]

as the no-alloc tests are now conditional on the value of an environment variable.

I'm missing why that's the case. I don't see anything in the source code looking at that env var?

[nsajko]

I'm missing why that's the case.

A code coverage-enabled build, as opposed to a production build, makes constant folding less reliable: JuliaLang/julia#49978. Thus the test suite needs to be able to tell whether the build is a production build or not. If not, the no-alloc tests are disabled.

I don't see anything in the source code looking at that env var?

It's only used from the test suite to disable the no-alloc tests.

[giordano]

Then check Bool(Base.JLOptions().code_coverage) instead of introducing an extra environment variable? I do something similar, but for bounds checking, in another package: https://github.com/JuliaIPU/IPUToolkit.jl/blob/66241df623317927561ba3985c4f1e907b7bd928/test/compiler.jl#L3-L4. Then you can do something like

diff --git a/.github/workflows/UnitTests.yml b/.github/workflows/UnitTests.yml
index 3d3d222..4175491 100644
--- a/.github/workflows/UnitTests.yml
+++ b/.github/workflows/UnitTests.yml
@@ -28,6 +28,9 @@ jobs:
           - ubuntu-latest
           - macos-latest
           - windows-latest
+        code_coverage:
+          - true
+          - false
 
     runs-on: ${{ matrix.os }}
 
@@ -39,6 +42,8 @@ jobs:
         version: ${{ matrix.julia_version }}
     - uses: julia-actions/cache@v2
     - uses: julia-actions/julia-runtest@v1
+      with:
+        coverage: ${{ matrix.code_coverage }}
     - uses: julia-actions/julia-processcoverage@v1
     - uses: codecov/codecov-action@v4
       with:

to test all configurations with code coverage both on and off. Alternatively, you can do

diff --git a/.github/workflows/UnitTests.yml b/.github/workflows/UnitTests.yml
index 3d3d222..adcf6c7 100644
--- a/.github/workflows/UnitTests.yml
+++ b/.github/workflows/UnitTests.yml
@@ -28,6 +28,13 @@ jobs:
           - ubuntu-latest
           - macos-latest
           - windows-latest
+        code_coverage:
+          - true
+        include:
+          - julia_version: "~1.11.0-0"
+            julia_arch: x64
+            os: ubuntu-latest
+            code_coverage: false
 
     runs-on: ${{ matrix.os }}
 
@@ -39,6 +46,8 @@ jobs:
         version: ${{ matrix.julia_version }}
     - uses: julia-actions/cache@v2
     - uses: julia-actions/julia-runtest@v1
+      with:
+        coverage: ${{ matrix.code_coverage }}
     - uses: julia-actions/julia-processcoverage@v1
     - uses: codecov/codecov-action@v4
       with:

to add a single configuration with code coverage disabled (maybe doubling the number of CI jobs is a bit too much).

[nsajko]

I considered just using Base.JLOptions. Two problems:

1. It wouldn't be robust. The Julia test process could have been started with some other configuration that would also cause the no-alloc tests to fail, such as --compile=no or --compile=min. Trying to check all the possible compilation options, any of which could signify that the build isn't a production build, seems like a fool's errand.

2. It's not public

[giordano]

I'm still confused by how BUILD_IS_PRODUCTION_BUILD will be defined though: if we can't control the precise conditions under which the build is "production-like", how'd the variable be set in the first place?

[nsajko]

It would be set in .github/workflows/UnitTests.yml, only for certain jobs.

[giordano]

Yeah, but what are these certain jobs?

[nsajko]

I think only a single CI run with code coverage is necessary, no? So I think one of these two alternatives would be best:

• Have a single run with code coverage, and all other without and marked as production

• Have a single run with code coverage, a single without code coverage and marked as production, and all other with --compile=min

The second option seems like the best because it seems very comprehensive, but it'd also shorten the total run time because testing with --compile=min is very fast (apparently most of the testing run time is compilation otherwise)

[nsajko]

Wasn't able to set up local testing of Github actions properly, hoping this passes.

NB: Although this PR allows constructing FixedSizeArray wrapping AtomicMemory, e.g. FixedSizeMatrix{Int,AtomicMemory{Int}}(undef, 2, 3), many operations on such FixedSizeArray fail, as AtomicMemory doesn't support setindex!(?). TBH I'm not even sure if a public API for mutating AtomicMemory exists as of yet. Perhaps with one of the unsafe_* functions.

[oscardssmith]

it does support setindex!, but only actual calls to setindex! since you also need to pass the type of atomic. we don't yet have a nice bracket syntax for that

[kalmarek]

after JuliaLang/julia#54707 you could write @atomic mem[idx] = val.
It transforms this to Base.setindex_atomic!(mem, :sequentially_consistent, val, idx).

[nsajko]

Sadly your PR won't land in v1.11 as far as I understand, so I suppose we should merge this PR as it is. After I check that all tests are still passing for nightly Julia, I guess.

[kalmarek]

No, it will not, but I'm thinking of forking this into functionality into a tiny package, that would allow this on 1.11

[giordano]

@nsajko anything else to do on this PR?

[giordano]

@nsajko I'm inclined to merge this PR in a couple of days, if you don't have plans to make further changes.