-
Notifications
You must be signed in to change notification settings - Fork 69
webGPU resources #180
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
base: 12-17-refactor_buffer_storage_split_split_split
Are you sure you want to change the base?
webGPU resources #180
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
}) | ||
); | ||
|
||
passEncoder.dispatchWorkgroups(size / 4 / 64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workgroup dispatch calculation size / 4 / 64
should be wrapped in Math.ceil()
to prevent dropping elements when the size is not perfectly divisible by the workgroup size. Without rounding up, the last partial workgroup may be skipped, leaving some elements unprocessed.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
@compute @workgroup_size(64) | ||
//TODOD HERE | ||
` }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shader module is missing its main
function implementation after the @compute
workgroup declaration. The code as written will fail to compile. Consider implementing the core compute shader logic here before merging.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
7237d17
to
4bb8cf9
Compare
1e1ae8e
to
830811c
Compare
4bb8cf9
to
61337d0
Compare
830811c
to
a23a2c3
Compare
61337d0
to
eef33f1
Compare
a23a2c3
to
b9dc76b
Compare
eef33f1
to
38c301d
Compare
b9dc76b
to
83f360f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 83f360f in 2 minutes and 19 seconds. Click for details.
- Reviewed
509
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/webGPU/forests.ts:77
- Draft comment:
Typo in shader comment: 'TODOD' should likely be 'TODO'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =70%
<= threshold70%
This comment points out a typo in a comment within the code. Correcting typos in comments can improve code readability and maintainability, which aligns with the rule of using descriptive variable and constant names. However, it doesn't directly impact the functionality or performance of the code. Still, it's a minor but valid suggestion for code quality improvement.
2. src/webGPU/buffertools.ts:79
- Draft comment:
Typo: In the comment on line 78, "cast it to uint32array" should use the correct capitalization as "Uint32Array". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 70% While technically correct that Uint32Array is the proper capitalization, this is just about the text in a code comment, not about the actual code functionality. The code itself on line 80 uses the correct Uint32Array capitalization. Comments are meant to convey meaning, and the lowercase version still effectively communicates the intent. This feels like an extremely minor nitpick that doesn't improve code quality. The proper capitalization could be considered part of maintaining technical accuracy in documentation. Consistent and correct terminology usage can help prevent confusion. While technical accuracy is good, this change would have no impact on code functionality or clarity - the meaning is clear either way. This falls under the rule of not making purely informative comments that don't require code changes. Delete this comment as it's an extremely minor documentation nitpick that doesn't impact code quality or functionality.
3. src/webGPU/forests.ts:77
- Draft comment:
Typo: 'TODOD HERE' seems to be a mistake. It should probably be 'TODO HERE'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 70% While this is technically a typo, it's in a TODO comment that's clearly marking incomplete code. The typo doesn't affect functionality and the entire shader module is incomplete. The comment is very minor and doesn't address any substantive issues. According to the rules, we should not make comments that are obvious or unimportant. The typo could be a sign of rushed or careless code writing, which might warrant extra attention to detail in review. TODOs in committed code could be worth flagging. While attention to detail is important, this is a trivial typo in a comment marking clearly incomplete code. The TODO itself indicates this is work in progress, and nitpicking spelling in such contexts isn't productive. Delete this comment as it points out an unimportant typo in a TODO marker for incomplete code, which doesn't warrant a formal review comment.
Workflow ID: wflow_mwTP0x2HA2ohU6b0
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
private async passThroughStagingBuffer(values: Uint32Array, bufferLocation: WebGPUBufferLocation) { | ||
// WebGPU | ||
const { buffer, offset, paddedSize } = bufferLocation; | ||
while (this.stagingBuffer.mapState !== 'unmapped') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Busy loop waiting for stagingBuffer
to be unmapped may block the main thread. Consider using a queuing mechanism to manage multiple mapping requests.
// If it's a typed array, we can just copy it directly. | ||
// cast it to uint32array | ||
const v2 = value; | ||
const data = new Uint32Array(v2.buffer, v2.byteOffset, v2.byteLength / 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting all TypedArray
values to Uint32Array
might be unsafe for non-32‐bit arrays. Ensure that the input type is appropriate.
buffer: this.buffers[0], | ||
offset: this.pointer, | ||
stride: 4, // meaningless here. | ||
byte_size: this.buffer_size - this.pointer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming: byte_size
uses snake_case while paddedSize
is camelCase. Please standardize new code to camelCase.
byte_size: this.buffer_size - this.pointer, | |
byteSize: this.buffer_size - this.pointer, |
} | ||
|
||
//@ts-expect-error foo | ||
private chooseNextFeatures(n = 32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove debugging console.log
and either use the parameter n
in chooseNextFeatures
or remove it if unnecessary.
} | ||
|
||
// hide the state in a global variable. | ||
const dumb: StatefulGPU[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename global variable dumb
to a more descriptive name (e.g., gpuInstances
) for clarity.
const defs = makeShaderDataDefinitions(shaderCode); | ||
const myUniformValues = makeStructuredView(defs.uniforms.myUniforms); | ||
myUniformValues.set({ | ||
objectSize: embeddingSize / 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the significance of dividing embeddingSize
by 32 when setting objectSize
in the uniform. A comment explaining this rationale would aid maintainability.
objectSize: embeddingSize / 32 | |
objectSize: embeddingSize / 32 // embeddingSize is in bits, but objectSize is in 32-bit words |
38c301d
to
169b197
Compare
83f360f
to
07bc4b3
Compare
Important
Adds WebGPU buffer management, TinyForest implementation, and compute shader preparation for enhanced GPU operations.
WebGPUBufferSet
class inbuffertools.ts
for managing GPU buffers with methods likeset()
andregister()
.createSingletonBuffer()
function for creating disposable GPU buffers.TinyForest
class inforests.ts
extendingStatefulGPU
for managing forest parameters and GPU buffers.countPipeline()
andchooseNextFeatures()
for compute pipeline setup and feature selection.prepareComputeShader()
function inlib.ts
for setting up compute shaders with buffer bindings.create_hamming_transform()
for creating transformations using Hamming distance in scatterplots.This description was created by
for 83f360f. You can customize this summary. It will automatically update as commits are pushed.