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

Erik/timerify #12334

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Erik/timerify #12334

wants to merge 22 commits into from

Conversation

erik-dunteman
Copy link
Contributor

What does this PR do?

Note that timerify records durations in two ways, through a Histogram and through sending events to PerformanceObserver, and this PR only addresses Histogram use.

The specific issue #9271 should remain open until PerformanceObserver is implemented.

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

Histogram: added plenty of tests in hdr_histogram.zig, all pass
Timerify: local sanity checks, below:

const { performance, createHistogram } = require("perf_hooks");

const fn = () => {
  console.log("hello world");
};

// test manual recording
let h = createHistogram();
h.record(100, 1);
h.record(200, 100);
h.record(1000, 1000);
h.record(2000, 1000);
h.record(3000, 1000);
h.record(5000, 1000);
h.record(1000000, 1);

// test add
let otherH = createHistogram();
otherH.record(1, 300);
h.add(otherH);

// test reset
h.reset();

// test timerify
let wrapped = performance.timerify(fn, { histogram: h });
for (let i = 0; i < 1000; i++) {
  wrapped();
}
console.log(h);

all behave as expected

"I wrote automated tests", though curious if all zig tests are ran in CI. we'll see.

  • I included a test for the new code, or existing tests cover it
  • I ran my tests locally and they pass (bun-debug test test-file-name.test)

Zig changes

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I included a test for the new code, or an existing test covers it
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

Types

  • I added TypeScript types for the new methods, getters, or setters -> matches node type interface

Copy link
Collaborator

@paperdave paperdave left a comment

Choose a reason for hiding this comment

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

overall alright. most of my worry is in the messy binding for creating the JSMap, and then a handful of simple to fix zig nitpicks. nice work on HDRHistogram.

also if youre writing documentation comments in zig code, that can use ///, ref

src/bun.js/bindings/JSMapObject.cpp Outdated Show resolved Hide resolved
src/js/builtins/Performance.ts Outdated Show resolved Hide resolved
src/js/node/perf_hooks.ts Outdated Show resolved Hide resolved
src/bun.js/node/node_perf_hooks_histogram_binding.zig Outdated Show resolved Hide resolved
src/bun.js/node/node_perf_hooks_histogram_binding.zig Outdated Show resolved Hide resolved
src/bun.js/node/node_perf_hooks_histogram_binding.zig Outdated Show resolved Hide resolved
bucket_count: u64,
counts: []u64,

const This = @This();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: this This is kind of silly to have. you can just refer to HDRHistogram, and in init you can use .{ instead of This{

.bucket_count = bucket_count,
.counts = counts,
.total_count = 0,
.min = 9223372036854776000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this start as highest_trackable_value? or std.math.maxInt(u64)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std.math.maxInt is probably better choice, whole point is to guarantee it gets overridden on next record

src/bun.js/node/hdr_histogram.zig Show resolved Hide resolved
src/bun.js/node/node_perf_hooks_histogram_binding.zig Outdated Show resolved Hide resolved
README.md Outdated
<a href="https://bun.sh"><img src="https://user-images.githubusercontent.com/709451/182802334-d9c42afe-f35d-4a7b-86ea-9985f73f20c3.png" alt="Logo" height=170></a>
</p>
<h1 align="center">Bun</h1>
# HdrHistogram_c
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol the readme got nuked. please revert

@erik-dunteman
Copy link
Contributor Author

Still need to

  • rebase to current main
  • look at Callconv stuff
  • implement bigInt getters

Copy link
Contributor

github-actions bot commented Jul 4, 2024

@erik-dunteman, your commit has failing tests :(

💻 2 failing tests Darwin x64 baseline

  • test/js/node/http/node-http.test.ts 1 failing
  • test/js/web/streams/streams.test.js 1 failing

💻 1 failing tests Darwin x64

  • test/js/node/http/node-http.test.ts 1 failing

🪟💻 5 failing tests Windows x64 baseline

  • test/cli/install/registry/bun-install-registry.test.ts 1 failing
  • test/cli/install/registry/bun-install-windowsshim.test.ts code 1
  • test/js/bun/util/fuzzy-wuzzy.test.ts STATUS_SEVERITY_ERROR
  • test/js/node/child_process/child_process.test.ts 1 failing
  • test/js/node/perf_hooks/perf_hooks.test.ts STATUS_SEVERITY_ERROR

🪟💻 5 failing tests Windows x64

  • test/cli/install/registry/bun-install-registry.test.ts 1 failing
  • test/cli/install/registry/bun-install-windowsshim.test.ts code 1
  • test/js/bun/util/fuzzy-wuzzy.test.ts STATUS_SEVERITY_ERROR
  • test/js/node/child_process/child_process.test.ts 1 failing
  • test/js/node/perf_hooks/perf_hooks.test.ts STATUS_SEVERITY_ERROR

View logs

@erik-dunteman
Copy link
Contributor Author

erik-dunteman commented Jul 6, 2024

Working on PerformanceObserver stuff now, but can make that a separate PR. This PR should be ready to go whenever CI passes and code review approved @Jarred-Sumner @paperdave

@Jarred-Sumner
Copy link
Collaborator

test/js/node/perf_hooks/perf_hooks.test.ts STATUS_INTEGER_DIVIDE_BY_ZERO
test/js/bun/util/fuzzy-wuzzy.test.ts STATUS_SEVERITY_ERROR

These are new test failures

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.

None yet

3 participants