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

url: add V8 Fast API for Blob RevokeObjectURL #54712

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tannal
Copy link
Contributor

@tannal tannal commented Sep 2, 2024

Description

This pull request adds a V8 Fast API implementation for the RevokeObjectURL method in the Blob API. This optimization aims to improve the performance of URL operations related to Blob objects.

Local Evaluation

With fast api.

revokeObjectURL: 147.683ms
Operations per second: 6771268

revokeObjectURL: 233.320ms
Operations per second: 4285953

revokeObjectURL: 148.093ms
Operations per second: 6752493

revokeObjectURL: 145.851ms
Operations per second: 6856293

revokeObjectURL: 146.318ms
Operations per second: 6834450

revokeObjectURL: 147.066ms
Operations per second: 6799684

revokeObjectURL: 148.605ms
Operations per second: 6729260

Without fast api.

revokeObjectURL: 234.338ms
Operations per second: 4267337

revokeObjectURL: 232.052ms
Operations per second: 4309385

revokeObjectURL: 231.606ms
Operations per second: 4317682

revokeObjectURL: 234.309ms
Operations per second: 4267869
const { performance, PerformanceObserver } = require('perf_hooks');

function revokeObjectURL(url) {
  URL.revokeObjectURL(url);
}

function runBenchmark(iterations = 1000000) {
  const url = URL.createObjectURL(new Blob(['hello world', '🍌🍌🍌']));
  performance.mark('start');
  
  for (let i = 0; i < iterations; i++) {
    revokeObjectURL(url);
  }
  
  performance.mark('end');
  performance.measure('revokeObjectURL', 'start', 'end');
}

const obs = new PerformanceObserver((list) => {
  const measurement = list.getEntries()[0];
  console.log(`revokeObjectURL: ${measurement.duration.toFixed(3)}ms`);
  
  const opsPerSecond = (1000000 / measurement.duration * 1000).toFixed(0);
  console.log(`Operations per second: ${opsPerSecond}`);

  performance.clearMarks();
  obs.disconnect();
});

obs.observe({ entryTypes: ['measure'] });

console.log("Running benchmark...");
runBenchmark();

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 2, 2024
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 37.83784% with 23 lines in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (0b6b2a4) to head (3c1f0ae).
Report is 1238 commits behind head on main.

Files with missing lines Patch % Lines
src/node_blob.cc 37.83% 18 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54712      +/-   ##
==========================================
+ Coverage   87.30%   87.60%   +0.30%     
==========================================
  Files         649      650       +1     
  Lines      182711   182963     +252     
  Branches    35031    35403     +372     
==========================================
+ Hits       159523   160293     +770     
+ Misses      16459    15933     -526     
- Partials     6729     6737       +8     
Files with missing lines Coverage Δ
src/node_external_reference.h 100.00% <ø> (ø)
src/node_blob.cc 72.99% <37.83%> (-3.83%) ⬇️

... and 83 files with indirect coverage changes

@avivkeller avivkeller added whatwg-url Issues and PRs related to the WHATWG URL implementation. needs-benchmark-ci PR that need a benchmark CI run. labels Sep 2, 2024
@jasnell
Copy link
Member

jasnell commented Sep 4, 2024

Generally looks good but needs linting and formatting updates.

@KhafraDev
Copy link
Member

@nodejs/url

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Tests are missing from this PR.

@tannal
Copy link
Contributor Author

tannal commented Nov 27, 2024

// Flags: --expose-internals --no-warnings --allow-natives-syntax
'use strict';

const { internalBinding } = require('internal/test/binding');
const assert = require('assert');
const common = require('../common');

function testFastPaths() {
    const url = URL.createObjectURL(new Blob(['hello world', '🍌🍌🍌']));
    URL.revokeObjectURL(url);
}

if (common.isDebug) {
    const { getV8FastApiCallCount } = internalBinding('debug');

    eval('%PrepareFunctionForOptimization(URL.revokeObjectURL)');
    testFastPaths();
    eval('%OptimizeFunctionOnNextCall(URL.revokeObjectURL)');
    testFastPaths();

    assert.strictEqual(getV8FastApiCallCount('blob.revokeObjectURL'), 1);
}

This test always failed. The TurboFan didn't call the fast method.
But the fast method did get called while running the benchmark code above (the code in #54712 (comment))
Am I missing something?

tannal@desktop:~/tannalwork/projects/node-blob-fast-api$ ./node_g --trace-opt --expose-internals --no-warnings --allow-natives-syntax ./test/parallel/test-whatwg-blob-revokeURL.js 
[marking 0x13b400e915d9 <JSFunction getNewKey (sfi = 0x247e0ccb65e9)> for optimization to MAGLEV, ConcurrencyMode::kConcurrent, reason: hot and stable]
[compiling method 0x13b400e915d9 <JSFunction getNewKey (sfi = 0x247e0ccb65e9)> (target MAGLEV), mode: ConcurrencyMode::kConcurrent]
[completed compiling 0x13b400e915d9 <JSFunction getNewKey (sfi = 0x247e0ccb65e9)> (target MAGLEV) - took 0.000, 0.526, 0.007 ms]
[marking 0x3de571bad7f9 <JSFunction getNewKey (sfi = 0x247e0ccb65e9)> for optimization to MAGLEV, ConcurrencyMode::kConcurrent, reason: hot and stable]
[compiling method 0x3de571bad7f9 <JSFunction getNewKey (sfi = 0x247e0ccb65e9)> (target MAGLEV), mode: ConcurrencyMode::kConcurrent]
[completed compiling 0x3de571bad7f9 <JSFunction getNewKey (sfi = 0x247e0ccb65e9)> (target MAGLEV) - took 0.000, 0.209, 0.005 ms]
[marking 0x34a89297dd21 <JSFunction (sfi = 0x26efb9430501)> for optimization to MAGLEV, ConcurrencyMode::kConcurrent, reason: hot and stable]
[compiling method 0x34a89297dd21 <JSFunction (sfi = 0x26efb9430501)> (target MAGLEV), mode: ConcurrencyMode::kConcurrent]
[completed compiling 0x34a89297dd21 <JSFunction (sfi = 0x26efb9430501)> (target MAGLEV) - took 0.000, 0.117, 0.003 ms]
[marking 0x13ccd0acc1d1 <JSFunction isPosixPathSeparator (sfi = 0x1a7d38fba549)> for optimization to MAGLEV, ConcurrencyMode::kConcurrent, reason: hot and stable]
[compiling method 0x13ccd0acc1d1 <JSFunction isPosixPathSeparator (sfi = 0x1a7d38fba549)> (target MAGLEV), mode: ConcurrencyMode::kConcurrent]
[completed compiling 0x13ccd0acc1d1 <JSFunction isPosixPathSeparator (sfi = 0x1a7d38fba549)> (target MAGLEV) - took 0.001, 0.145, 0.002 ms]
[marking 0x13ccd0acc251 <JSFunction normalizeString (sfi = 0x1a7d38fba5f9)> for optimization to MAGLEV, ConcurrencyMode::kConcurrent, reason: hot and stable]
[compiling method 0x0c3a3d8dd769 <JSFunction normalizeString (sfi = 0x1a7d38fba5f9)> (target MAGLEV), mode: ConcurrencyMode::kConcurrent]
[completed compiling 0x0c3a3d8dd769 <JSFunction normalizeString (sfi = 0x1a7d38fba5f9)> (target MAGLEV) - took 0.000, 0.563, 0.005 ms]
[manually marking 0x38f95568cab1 <JSFunction revokeObjectURL (sfi = 0x22baf1417f71)> for optimization to TURBOFAN, ConcurrencyMode::kSynchronous]
[compiling method 0x38f95568cab1 <JSFunction revokeObjectURL (sfi = 0x22baf1417f71)> (target TURBOFAN), mode: ConcurrencyMode::kSynchronous]
[completed compiling 0x38f95568cab1 <JSFunction revokeObjectURL (sfi = 0x22baf1417f71)> (target TURBOFAN) - took 0.011, 1.013, 0.025 ms]
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

0 !== 1

    at Object.<anonymous> (/home/tannal/tannalwork/projects/node-blob-fast-api/test/parallel/test-whatwg-blob-revokeURL.js:21:12)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
    at Module.load (node:internal/modules/cjs/loader:1317:32)
    at Module._load (node:internal/modules/cjs/loader:1127:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:166:5)
    at node:internal/main/run_main_module:30:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 0,
  expected: 1,
  operator: 'strictEqual'
}

Node.js v23.0.0-pre

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants