Sitelet https://github.com/nodejs/node/pull/54103
Skip to content

src: replace deprecated v8::FastApiTypedArray - #54103

Closed
targos wants to merge 1 commit into
nodejs:mainfrom
targos:v8-fast-rm-uint8array
Closed

targos wants to merge 1 commit into
nodejs:mainfrom
targos:v8-fast-rm-uint8array

Conversation

@targos

@targos targos commented Jul 29, 2024

Copy link
Copy Markdown
Member

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 29, 2024
@targos
targos marked this pull request as draft July 29, 2024 14:33
@targos

targos commented Jul 29, 2024

Copy link
Copy Markdown
Member Author

I'm not sure about the changes here, but I can update the other uses if they are correct.

@targos

targos commented Jul 29, 2024

Copy link
Copy Markdown
Member Author

Comment thread src/node_buffer.cc
@codecov

codecov Bot commented Jul 29, 2024 •

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.07%. Comparing base (01cf9bc) to head (895a3fc).
Report is 1625 commits behind head on main.

Files with missing lines Patch % Lines
src/node_buffer.cc 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54103      +/-   ##
==========================================
- Coverage   87.07%   87.07%   -0.01%     
==========================================
  Files         643      643              
  Lines      181583   181587       +4     
  Branches    34886    34887       +1     
==========================================
- Hits       158114   158109       -5     
+ Misses      16751    16748       -3     
- Partials     6718     6730      +12     
Files with missing lines Coverage Δ
src/node_external_reference.h 100.00% <ø> (ø)
src/node_buffer.cc 74.01% <0.00%> (-0.21%) ⬇️

... and 27 files with indirect coverage changes

@ronag

ronag commented Jul 29, 2024

Copy link
Copy Markdown
Member

Seems significantly slower. I would propose we stick with the deprecated api until there is a faster alternative or until it's been removed.

@ronag

ronag commented Jul 29, 2024

Copy link
Copy Markdown
Member

I had similar results with the copy pr

Comment thread src/node_buffer.cc

int32_t FastIndexOfNumber(v8::Local<v8::Value>,
const FastApiTypedArray<uint8_t>& buffer,
v8::Local<v8::Object> buffer,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is actually slower in V8 v12.8 (by a significant margin).

@targos targos added the blocked PRs that are blocked by other issues or PRs. label Aug 1, 2024
@ronag ronag mentioned this pull request Oct 21, 2024
2 of 7 tasks
@targos targos closed this Feb 15, 2025
@targos
targos deleted the v8-fast-rm-uint8array branch February 15, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked PRs that are blocked by other issues or PRs. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants