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

perf: use samplesLength instead of multiple use samples.length #59

Merged
merged 6 commits into from
Oct 11, 2023

Conversation

Dunqing
Copy link
Contributor

@Dunqing Dunqing commented Oct 8, 2023

No description provided.

@Dunqing
Copy link
Contributor Author

Dunqing commented Oct 8, 2023

If the number of samples is very large, calling sort() can be very expensive.

src/task.ts Outdated
this.runs += 1;
totalTime += taskTime;
samples.push(taskTime);
if (taskTime > max) max = taskTime;
if (taskTime < min) min = taskTime;
Copy link
Member

Choose a reason for hiding this comment

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

what do these lines do exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get the max and min values, This way we don't have to call sort() again.

@Aslemammad
Copy link
Member

Do you have any benchmark that approves these changes?

@Dunqing
Copy link
Contributor Author

Dunqing commented Oct 8, 2023

Here are the results of my benchmark. 2.x faster than before

┌─────────┬─────────────────┬─────────┬────────────────────┬──────────┬─────────┐
│ (index) │    Task Name    │ ops/sec │ Average Time (ns)  │  Margin  │ Samples │
├─────────┼─────────────────┼─────────┼────────────────────┼──────────┼─────────┤
│    0    │ 'before change''0'   │ 1024660996.7470169 │ '±4.00%' │   50    │
│    1    │ 'after change''2'   │ 405250253.31020355 │ '±1.01%' │   50    │
└─────────┴─────────────────┴─────────┴────────────────────┴──────────┴─────────┘```

These codes for benchmark

import { Bench as BeforeBench } from 'oldtinybench';
import { Bench } from 'tinybench';

const bench = new Bench({
  iterations: 50,
});

const testFunction = async (BenchC: typeof Bench) => {
  const bb = new BenchC({ time: 100 });

  bb
    .add('faster task', () => {
      // console.log('I am faster');
    })
    .add('slower task', async () => {
      await new Promise((r) => setTimeout(r, 1)); // we wait 1ms :)
      // console.log('I am slower');
    })
    .todo('unimplemented bench');

  await bb.run();
};

bench
  .add('before change', () => testFunction(BeforeBench))
  .add('after change', () => testFunction(Bench));

await bench.warmup();
await bench.run();

console.table(bench.table());

src/task.ts Outdated
const period = totalTime / this.runs;
const hz = 1000 / period;

// benchmark.js: https://github.com/bestiejs/benchmark.js/blob/42f3b732bac3640eddb3ae5f50e445f3141016fd/benchmark.js#L1912-L1927
const simplesLength = samples.length;
Copy link
Member

Choose a reason for hiding this comment

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

samplesLength

src/task.ts Outdated
@@ -57,6 +57,8 @@ export default class Task extends EventTarget {
async run() {
this.dispatchEvent(createBenchEvent('start', this));
let totalTime = 0; // ms
let min = Infinity;
let max = -Infinity;
Copy link
Member

Choose a reason for hiding this comment

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

if we want to get the min and max, we need to return them actually, and when you do, add it to the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

And aren't they in the wrong order? min should be -Infinity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we want to get the min and max, we need to return them actually, and when you do, add it to the documentation.

I do not understand what you're saying. There were min and max in the first place just a different way to get them.

And aren't they in the wrong order? min should be -Infinity.

I think you may have misunderstood. If min is -Infinity it means that there is no value smaller than this min

Copy link
Member

Choose a reason for hiding this comment

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

Oh I get it now.

Copy link
Member

@Aslemammad Aslemammad Oct 8, 2023

Choose a reason for hiding this comment

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

I do not understand what you're saying. There were min and max in the first place just a different way to get them.

Yes, we used to get them by samples[samples.length] for instance, but since we do not sort now, we need to return them along other results like mean, variance, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not understand what you're saying. There were min and max in the first place just a different way to get them.

Yes, we used to get them by samples[samples.length] for instance, but since we do not sort now, we need to return them along other results like mean, variance, ...

Oh, I got it. Looks like we still need to call sort() otherwise the result will be incorrect

@Dunqing Dunqing changed the title perf: avoid calling sort() and multiple use samples.length perf: use samplesLength instead of multiple use samples.length Oct 8, 2023
@Aslemammad
Copy link
Member

based on the new changes, can you send the new benchmarks? I'd appreciate it.

@Dunqing
Copy link
Contributor Author

Dunqing commented Oct 8, 2023

based on the new changes, can you send the new benchmarks? I'd appreciate it.

Looks like the changes have little difference in performance

➜ npx tsx  ./src/test-tinybench.ts
┌─────────┬─────────────────┬─────────┬────────────────────┬──────────┬─────────┐
│ (index) │    Task Name    │ ops/sec │ Average Time (ns)  │  Margin  │ Samples │
├─────────┼─────────────────┼─────────┼────────────────────┼──────────┼─────────┤
│    0    │ 'before change''0'   │ 1023757644.1144943 │ '±3.63%' │   50    │
│    1    │ 'after change''0'   │ 1019888030.8961868 │ '±3.74%' │   50    │
└─────────┴─────────────────┴─────────┴────────────────────┴──────────┴─────────┘

@Aslemammad
Copy link
Member

Let's remove the sort as I think it's better to not have it, but please also mention that in the docs. I believe the min/max approach is much better.

@Dunqing
Copy link
Contributor Author

Dunqing commented Oct 9, 2023

Let's remove the sort as I think it's better to not have it, but please also mention that in the docs. I believe the min/max approach is much better.

p75, p99, p995, p999 depend on sort(). I haven't figured out a way around that yet.

@Aslemammad Aslemammad merged commit f52f027 into tinylibs:main Oct 11, 2023
3 checks passed
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