donmccurdy / glTF-Transform

glTF 2.0 SDK for JavaScript and TypeScript, on Web and Node.js.
https://gltf-transform.dev
MIT License
1.3k stars 145 forks source link

fix(core): Fix invalid state of accessor.normalized after clone #1275

Closed donmccurdy closed 4 months ago

donmccurdy commented 4 months ago

Currently the accessor implementation stores references to functions that encode/decode normalized vertex values. When cloning an accessor, those functions held references to their previous scope, and looked up the previous accessor's normalization state.

This PR changes and simplifies the implementation to avoid storing any unnecessary state. The previous implementation was a premature optimization that (as shown by the benchmarks below) did little or nothing. There might be a small regression in join() performance, but that's ongoing work for #1253.

Changes

Benchmark - before

┌─────────┬───────────────┬─────────┬────────────────────┬──────────┬─────────┐
│ (index) │   Task Name   │ ops/sec │ Average Time (ns)  │  Margin  │ Samples │
├─────────┼───────────────┼─────────┼────────────────────┼──────────┼─────────┤
│    0    │  'clone::sm'  │  '78'   │ 12718226.924727235 │ '±2.53%' │   79    │
│    1    │  'clone::md'  │  '18'   │  53303135.9910965  │ '±3.31%' │   19    │
│    2    │ 'create::sm'  │  '135'  │ 7357290.743904955  │ '±3.39%' │   136   │
│    3    │ 'create::md'  │  '33'   │ 30239656.813004438 │ '±3.43%' │   34    │
│    4    │ 'dispose::md' │ '2,063' │ 484532.1858229563  │ '±3.21%' │  2064   │
│    5    │  'join::sm'   │  '33'   │ 30231020.916910734 │ '±4.53%' │   34    │
│    6    │  'join::md'   │   '1'   │ 667896770.6918716  │ '±0.96%' │   10    │
│    7    │  'weld::sm'   │ '1,036' │ 965071.4626192702  │ '±1.77%' │  1037   │
│    8    │  'weld::md'   │  '15'   │  65537336.0812664  │ '±3.01%' │   16    │
└─────────┴───────────────┴─────────┴────────────────────┴──────────┴─────────┘

Benchmark - after

┌─────────┬───────────────┬─────────┬────────────────────┬──────────┬─────────┐
│ (index) │   Task Name   │ ops/sec │ Average Time (ns)  │  Margin  │ Samples │
├─────────┼───────────────┼─────────┼────────────────────┼──────────┼─────────┤
│    0    │  'clone::sm'  │  '78'   │ 12777751.054944873 │ '±2.65%' │   79    │
│    1    │  'clone::md'  │  '18'   │ 52808026.30123339  │ '±2.90%' │   19    │
│    2    │ 'create::sm'  │  '135'  │  7378732.41501133  │ '±3.60%' │   137   │
│    3    │ 'create::md'  │  '33'   │ 29939408.004283905 │ '±2.62%' │   34    │
│    4    │ 'dispose::md' │ '2,084' │ 479733.5695019729  │ '±3.22%' │  2085   │
│    5    │  'join::sm'   │  '28'   │ 35120684.052335806 │ '±4.73%' │   29    │
│    6    │  'join::md'   │   '1'   │ 771483329.2603493  │ '±0.80%' │   10    │
│    7    │  'weld::sm'   │ '1,042' │ 959606.5411974577  │ '±1.80%' │  1043   │
│    8    │  'weld::md'   │  '15'   │ 64910143.24873686  │ '±3.33%' │   16    │
└─────────┴───────────────┴─────────┴────────────────────┴──────────┴─────────┘
donmccurdy commented 4 months ago

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.