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

use a shared object with multiple length keys to store regl buffers #140

Conversation

bmschmidt
Copy link
Collaborator

@bmschmidt bmschmidt commented Sep 9, 2024

Previously deepscatter had something called a TileBufferManager. This doesn't make a huge amount of sense, and had a concrete cost; if two tiles used the same underlying Arrow memory for a column (say, 2**16 floating point zeros), then we'd use different memory on the GPU for them.

This PR revamps that storage so that it uses the new TupleMap class. There is a single shared buffer manager across the whole plot, which uses a key that includes the tile itself and then the string name of the field desired (which is now actually string[], for help in the next PR.)

This map associates each column with its underlying Arrow vector memory, and it is that memory that is used as a key to the GPU buffers. This is a little weird, but the two-step operation means taht we can re-use memory more efficiently.

At some later point I'll more fully build on this. One goal is that for four types of floating point data we can always use a single webGL buffer instead of re-allocating them ad-infinitum:

  1. All-false (or all-zero in float32 land)
  2. All-true (or all-one in float32 land)
  3. Nth-item-true, rest false (by creating a big array with a one in the middle at position K, and starting the view of that buffer at position K-N.
  4. Nth-item-false, rest true (inverse of number 3).

Important

Refactor buffer management by replacing TileBufferManager with a shared BufferManager for efficient GPU memory usage across tiles.

  • Buffer Management:
    • Replaced TileBufferManager with a shared BufferManager using TupleMap for efficient GPU memory management.
    • BufferManager uses a key with tile and field name for buffer management.
    • Introduced BufferManager.get and BufferManager.ready for buffer retrieval and readiness checks.
  • Renderer Updates:
    • Updated ReglRenderer to use a single BufferManager instance.
    • Removed _buffer_manager references from Tile class.
  • TileDrawProps:
    • Updated to include tile and count properties.
  • Memory Efficiency:
    • Improved memory efficiency by reusing GPU buffers for common data patterns.
  • Misc:
    • Removed 'observable10' from color schemes in ColorChange.svelte.

This description was created by Ellipsis for 5903dbd. It will automatically update as commits are pushed.

Copy link
Collaborator Author

bmschmidt commented Sep 9, 2024

@bmschmidt bmschmidt marked this pull request as ready for review September 9, 2024 15:58
@bmschmidt bmschmidt requested a review from RLesser September 16, 2024 20:10
@bmschmidt bmschmidt force-pushed the 09-07-add_tuplemap_class branch from 290e0fe to dd9bc11 Compare September 19, 2024 19:00
@bmschmidt bmschmidt force-pushed the 09-09-use_a_shared_object_with_multiple_length_keys_to_store_regl_buffers branch from aa964e6 to 2dac6bb Compare September 19, 2024 19:00
private _integer_buffer?: Buffer;

constructor(renderer: ReglRenderer) {
this.regl = renderer.regl;
this.renderer = renderer;
// Reuse the same buffer for all `ix_in_tile` keys, because
Copy link

Choose a reason for hiding this comment

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

The logic for creating a buffer for 'ix_in_tile' is repeated in both the constructor and the create_regl_buffer method. Consider refactoring to avoid repetition and adhere to the DRY principle.

* @returns A Float32-converted version of the data in the column
* suitable to be dropped onto a webGL buffer.
*/
convertToGlFormat(column: Vector<DS.SupportedArrowTypes>): Float32Array {
Copy link

Choose a reason for hiding this comment

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

The convertToGlFormat method is complex and handles various data types. Consider adding comments to explain the logic, especially for timestamp conversion and dictionary handling, to improve code readability and maintainability.

@bmschmidt bmschmidt force-pushed the 09-07-add_tuplemap_class branch from dd9bc11 to cd93751 Compare September 20, 2024 02:54
@bmschmidt bmschmidt force-pushed the 09-09-use_a_shared_object_with_multiple_length_keys_to_store_regl_buffers branch from 2dac6bb to c7a8537 Compare September 20, 2024 02:55
constructor(regl: Regl, tile: Tile, renderer: ReglRenderer) {
this.tile = tile;
this.regl = regl;
private bufferMap: WeakMap<ArrayBufferView, DS.BufferLocation> = new Map();
Copy link

Choose a reason for hiding this comment

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

Using WeakMap for bufferMap is incorrect as WeakMap should be used for objects, not ArrayBufferView. Consider using a regular Map instead.

@bmschmidt bmschmidt changed the base branch from 09-07-add_tuplemap_class to graphite-base/140 September 20, 2024 03:08
@bmschmidt bmschmidt force-pushed the 09-09-use_a_shared_object_with_multiple_length_keys_to_store_regl_buffers branch from c7a8537 to 2315827 Compare September 20, 2024 03:23
@bmschmidt bmschmidt changed the base branch from graphite-base/140 to main September 20, 2024 03:24
@bmschmidt bmschmidt force-pushed the 09-09-use_a_shared_object_with_multiple_length_keys_to_store_regl_buffers branch from 2315827 to d175c73 Compare September 20, 2024 03:24
@@ -123,7 +120,8 @@ export class Tile {

deleteColumn(colname: string) {
if (this._batch) {
this._buffer_manager?.release(colname);
console.warn('Deleting column from tile doesnt free GPU memory');
Copy link

Choose a reason for hiding this comment

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

The deleteColumn method does not free GPU memory, which could lead to memory leaks. Consider implementing GPU memory release when a column is deleted.

@bmschmidt bmschmidt force-pushed the 09-09-use_a_shared_object_with_multiple_length_keys_to_store_regl_buffers branch from d175c73 to fb591fb Compare September 20, 2024 15:32
Copy link
Contributor

RLesser commented Sep 25, 2024

These two PRs need to get rebased in some form right? I'm seeing "Trunk branch locked" in graphite

Copy link
Collaborator Author

Yeah

Copy link
Collaborator Author

It's weird though they used to be on dev which is locked, but now they're against main but graphite seems unaware.

@bmschmidt bmschmidt force-pushed the 09-09-use_a_shared_object_with_multiple_length_keys_to_store_regl_buffers branch from fb591fb to 9cdf75f Compare October 1, 2024 00:36
@bmschmidt bmschmidt force-pushed the 09-09-use_a_shared_object_with_multiple_length_keys_to_store_regl_buffers branch from 9cdf75f to 5903dbd Compare October 1, 2024 00:48
@@ -45,6 +41,8 @@ import { Color } from './aesthetics/ColorAesthetic';
import { StatefulAesthetic } from './aesthetics/StatefulAesthetic';
import { Filter, Foreground } from './aesthetics/BooleanAesthetic';
import { ZoomTransform } from 'd3-zoom';
import { TupleMap } from './utilityFunctions';
import { buffer } from 'd3';
Copy link

Choose a reason for hiding this comment

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

The import 'buffer' from 'd3' is unused and can be removed to clean up the code.

@bmschmidt bmschmidt closed this Oct 24, 2024
@bmschmidt
Copy link
Collaborator Author

Merged through other mans

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants