refactor: support Uint8Array - #260
Conversation
|
|
||
| // Look for [0xff, 0xff, 0xff] metadata delimiter | ||
| if (delim[0] === 255 && delim[1] === 255 && delim[2] === 255) { | ||
| const offset = db.length - 3 - i; |
There was a problem hiding this comment.
nice, thanks for replacing the slice
There was a problem hiding this comment.
Pull request overview
This PR refactors the mmdb reader pipeline to operate on Uint8Array rather than Node-specific Buffer, enabling first-class usage in browser-like environments while keeping Node compatibility.
Changes:
- Replaced
Buffer-specific read APIs (readUInt*,readFloat*,toString('utf8')) withUint8Array-compatible helpers andTextDecoder. - Updated core reader components (
Reader,Decoder,Walker, metadata parsing) to acceptUint8Array. - Updated unit + integration tests and README examples to use
Uint8Array.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/reader/walker.ts | Switches node traversal reads to Uint8Array via new byte helpers. |
| src/metadata.ts | Refactors metadata marker/search logic to Uint8Array indexing. |
| src/ip.ts | Updates bit-level address access to accept Uint8Array. |
| src/ip.test.ts | Updates bitAt test to use Uint8Array. |
| src/index.ts | Changes Reader to accept Uint8Array and updates validation/cloning. |
| src/index.test.ts | Updates test DB loading to provide Uint8Array. |
| src/decoder.ts | Replaces Buffer read methods with bytes.ts helpers and TextDecoder. |
| src/decoder.test.ts | Updates decoder tests to build inputs/expectations as Uint8Array. |
| src/bytes.ts | Introduces Uint8Array helpers for integer/float reads. |
| src/test/integration.test.ts | Updates integration expectations for decoded byte fields to Uint8Array. |
| README.md | Updates public docs to describe Uint8Array usage (browser + Node). |
Comments suppressed due to low confidence (1)
src/bytes.ts:36
readFloat32/readFloat64allocate a newDataViewon every call. These functions can be called many times during decoding, so this adds avoidable per-value allocations. Caching theDataViewperUint8Arrayavoids repeated object creation while keeping the same semantics.
export const readFloat32 = (bytes: Uint8Array, offset: number): number =>
new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength).getFloat32(
offset,
false
);
export const readFloat64 = (bytes: Uint8Array, offset: number): number =>
new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength).getFloat64(
offset,
false
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Could you please rebase with master, I've added a benchmark to avoid nasty regressions |
| ); | ||
| } | ||
| this.db = db; | ||
| this.db = new Uint8Array(db.buffer, db.byteOffset, db.byteLength); |
There was a problem hiding this comment.
Why are you doing it? Is it really necessary?
There was a problem hiding this comment.
I will double-check again. I was worried about potentially mutating the input, and also just normalizing the input (turning the Buffer into a plain Uint8Array), but this might indeed not be necessary. Will get back after I double-check.
There was a problem hiding this comment.
Dropped the Uint8Array clone and updated the test cases in fdc0ddf (this PR).
| constructor(db: Uint8Array, baseOffset = 0, cache: Cache = noCache) { | ||
| utils.assert(Boolean(db), 'Database buffer is required'); | ||
| this.db = db; | ||
| this.db = new Uint8Array(db.buffer, db.byteOffset, db.byteLength); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Dropped the Uint8Array clone and updated the test cases in fdc0ddf (this PR).
|
Hmm, there's about 20% performance degradation with this change. Do you have a feel of where it could be coming from? My local run: master: 157k lookups/sec |
Weird. I will look into this. In the mean time I will also fix unit test type mismatch issue here. |
|
@runk I have optimized db read in However, the short string |
|
Hmm.. I'm wondering if a thin wrapper on top of byte array source can be the solution here. It'd maintain the baseline for nodejs env and do the trick for browser env - I'm talking about two adapters with simialr interface |
The PR refactors the library to accept
Uint8Array.Existing
Bufferusage won't be broken, asBufferis a subclass ofUint8Array, and feross'sbufferpolyfill is also built uponUint8Array.A new
bytes.tsis introduced to bridge the gap in support forreadUint16,readUint32,readInt32,readFloat32, andreadFloat64for Uint8Array.