Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Validate Circular Buffer Capacity #91

Merged
merged 8 commits into from
Nov 26, 2023

Conversation

bushidocodes
Copy link
Contributor

glennj discovered a few errors in the circular buffer exercise while working through the track. This PR addresses all three.

This does add new tests and potentially break existing solutions. I would appreciate guidance on how best to merge this. I will be away from computers tomorrow during US Thanksgiving, so please feel free to merge on my behalf.

Error 1: The (memory 1) initializes the linear memory with 1 64KiB page, but it does not provide a maximum cap on what it can grow to. This means the linear memory can grow up to a full 32-bit address space. In C terms, that means you can store up to UINT32_MAX / sizeof(uint32_t) elements before filling up. I believe my intent here was that the memory instruction should be (memory 1 1), meaning "initialized with 1 64KiB page and do not allow it to grow beyond."

Error 2: The comment "capacity of the circular buffer between 0 and 1024 in order to fit in a single WebAssembly page" is wrong because wasm pages are 64Kib, not 4KiB. It should read "capacity of the circular buffer between 0 and 16,384 in order to fit in a single 64KiB WebAssembly page"

Error 3: Missing tests

@bushidocodes bushidocodes added the x:size/small Small amount of work label Nov 23, 2023
@bushidocodes bushidocodes removed the request for review from ErikSchierboom November 23, 2023 04:05
@bushidocodes bushidocodes marked this pull request as draft November 23, 2023 04:05
@bushidocodes
Copy link
Contributor Author

Converted to draft to have more time to solicit feedback.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

This does add new tests and potentially break existing solutions. I would appreciate guidance on how best to merge this. I will be away from computers tomorrow during US Thanksgiving, so please feel free to merge on my behalf.

First of all, it's great that you're thinking about this! In this case, I think it's fine to merge as there are relatively few solutions to this exercise, so we won't take a big hit on our infrastructure.

@@ -1,12 +1,13 @@
(module
(memory 1)
;; Cap the memory at a single 64KiB page
(memory 1 1)
Copy link
Member

Choose a reason for hiding this comment

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

Side note: is this (capping the memory) something we want to do for other exercises too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can! It's mostly useful for preventing errors in loops that call memory.grow, so it's particularly important here. No harm in doing this more widely though. I'll do that as a follow-up PR.

@bushidocodes
Copy link
Contributor Author

I've modified this exercise to require the student to conditionally invoke memory.grow on the circular buffer's linear memory depending on the desired capacity based on feedback from @glennj.

@bushidocodes bushidocodes added x:size/medium Medium amount of work and removed x:size/small Small amount of work labels Nov 25, 2023
(i32.const 0)
) (else
;; memory.grow returns old size on success or -1 on failure
(memory.grow (i32.div_s (i32.sub (local.get $newCapacity) (i32.const 1)) (i32.const 16384)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want (newCap - 1) / 16384 or (newCap / 16384) - 1 here? I think the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latter actually ends up allocating too few pages I believe due to integer division acting like a floor:

x idiv y ~= floor(x/y)

i.e. when we want to allocate a circular buffer that holds one more than we can fit in 1 page.

(newCap - 1) / 16384
((16385 - 1) idiv 16384) => (16384 idiv 16384) => 1

(newCap / 16384) - 1
((16385 idiv 16384) - 1) => (1 - 1) => 0

I checked this by modifying the solution to (memory.grow (i32.sub (i32.div_s (local.get $newCapacity) (i32.const 16384)) (i32.const 1))) and got the following:

    ✓ initializing negative capacity should error (2 ms)
    ✓ initializing capacity of 0 i32s should result in a linear memory with 1 page (1 ms)
    ✓ initializing capacity of 16384 i32s should result in a linear memory with 1 page
    ✕ initializing capacity of 16385 i32s should result in a linear memory with 2 pages (1 ms)
    ✓ initializing capacity of 32768 i32s should result in a linear memory with 2 pages (1 ms)
    ✕ initializing capacity of 32769 i32s should result in a linear memory with 3 pages
    ✓ initializing capacity of 49152 i32s should result in a linear memory with 3 pages
    ✕ initializing capacity of 49153 i32s should result in a linear memory with 4 pages
    ✓ initializing capacity of 65536 i32s should result in a linear memory with 4 pages
    
      ● CircularBuffer › initializing capacity of 16385 i32s should result in a linear memory with 2 pages

    expect(received).toEqual(expected) // deep equality

    Expected: 2
    Received: 1

      47 |   test("initializing capacity of 16385 i32s should result in a linear memory with 2 pages", () => {
      48 |     expect(currentInstance.exports.init(16385)).toEqual(0);
    > 49 |     expect(currentInstance.exports.mem.buffer.byteLength / 65536).toEqual(2);
         |                                                                   ^
      50 |   });
      51 |
      52 |   test("initializing capacity of 32768 i32s should result in a linear memory with 2 pages", () => {

      at Object.toEqual (circular-buffer.spec.js:49:67)

  ● CircularBuffer › initializing capacity of 32769 i32s should result in a linear memory with 3 pages

    expect(received).toEqual(expected) // deep equality

    Expected: 3
    Received: 2

      57 |   test("initializing capacity of 32769 i32s should result in a linear memory with 3 pages", () => {
      58 |     expect(currentInstance.exports.init(32769)).toEqual(0);
    > 59 |     expect(currentInstance.exports.mem.buffer.byteLength / 65536).toEqual(3);
         |                                                                   ^
      60 |   });
      61 |
      62 |   test("initializing capacity of 49152 i32s should result in a linear memory with 3 pages", () => {

      at Object.toEqual (circular-buffer.spec.js:59:67)

  ● CircularBuffer › initializing capacity of 49153 i32s should result in a linear memory with 4 pages

    expect(received).toEqual(expected) // deep equality

    Expected: 4
    Received: 3

      67 |   test("initializing capacity of 49153 i32s should result in a linear memory with 4 pages", () => {
      68 |     expect(currentInstance.exports.init(49153)).toEqual(0);
    > 69 |     expect(currentInstance.exports.mem.buffer.byteLength / 65536).toEqual(4);
         |                                                                   ^
      70 |   });
      71 |
      72 |   test("initializing capacity of 65536 i32s should result in a linear memory with 4 pages", () => {

      at Object.toEqual (circular-buffer.spec.js:69:67)

exercises/practice/circular-buffer/circular-buffer.wat Outdated Show resolved Hide resolved
exercises/practice/circular-buffer/.docs/hints.md Outdated Show resolved Hide resolved
@bushidocodes bushidocodes marked this pull request as ready for review November 25, 2023 17:11
@bushidocodes bushidocodes requested a review from glennj November 25, 2023 17:16
Copy link
Contributor

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

@bushidocodes bushidocodes merged commit 3a409e5 into main Nov 26, 2023
2 checks passed
@bushidocodes bushidocodes deleted the fix/circular-buffer-max-cap branch November 26, 2023 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/medium Medium amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants