Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions src/jsc/JSValue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2593,6 +2593,53 @@ impl JSValue {
index => Some(index as u32),
}
}
/// The vector-storage half of `next_present_index`: the smallest index
/// `>= start` that the array's butterfly vector holds. Never reads the
/// sparse map, so asking once per run of holes stays linear in what the
/// array stores.
/// Asserts `self` is a `JSArray` (`Array` or `DerivedArray`).
pub fn next_present_vector_index(self, start: u32) -> Option<u32> {
debug_assert!(self.is_cell() && self.js_type().is_array());
unsafe extern "C" {
safe fn Bun__JSArray__nextPresentVectorIndex(this: JSValue, start: u32) -> u64;
}
match Bun__JSArray__nextPresentVectorIndex(self, start) {
u64::MAX => None,
index => Some(index as u32),
}
}
/// The indices in `start..end` that the array's sparse map holds,
/// ascending. The map is walked and sorted once, like JSC's
/// `getOwnIndexedPropertyNames`. The result is a copy: it stays valid, and
/// goes stale, when user code changes the array later.
/// Asserts `self` is a `JSArray` (`Array` or `DerivedArray`).
pub fn sorted_sparse_indices(self, start: u32, end: u32) -> Vec<u32> {
use bun_core::UnwrapOrOom as _;
debug_assert!(self.is_cell() && self.js_type().is_array());
unsafe extern "C" {
fn Bun__JSArray__copySortedSparseIndices(
this: JSValue,
start: u32,
end: u32,
out: *mut u32,
capacity: u32,
) -> u32;
}
let mut out: Vec<u32> = Vec::new();
loop {
let capacity = u32::try_from(out.capacity()).unwrap_or(u32::MAX);
// SAFETY: `out` has room for `capacity` u32s and C++ writes at most that many.
let count = unsafe {
Bun__JSArray__copySortedSparseIndices(self, start, end, out.as_mut_ptr(), capacity)
};
if count <= capacity {
// SAFETY: C++ initialized the first `count <= capacity` elements.
unsafe { out.set_len(count as usize) };
return out;
}
out.try_reserve_exact(count as usize).unwrap_or_oom();
}
}
/// `JSValue.getNameProperty` — the value's `.name` (function/class
/// name). Empty for empty/`undefined`/`null`.
pub fn get_name_property(self, global: &JSGlobalObject) -> JsResult<bun_core::String> {
Expand Down
106 changes: 83 additions & 23 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7103,22 +7103,16 @@ extern "C" bool Bun__JSArray__contiguousVectorIsStillValid(
return reinterpret_cast<const JSC::EncodedJSValue*>(butterfly->contiguous().data()) == expected;
}

// Smallest own present index of a JSArray that is >= `start`, or UINT64_MAX
// when every index from `start` to the end of the array is a hole. Mirrors the
// butterfly walk in JSObject::getOwnIndexedPropertyNames so the caller can skip
// a run of holes without probing each index of a huge sparse array.
extern "C" uint64_t Bun__JSArray__nextPresentIndex(
JSC::EncodedJSValue encodedValue,
uint32_t start)
{
static constexpr uint64_t notFound = std::numeric_limits<uint64_t>::max();

JSC::JSArray* array = uncheckedDowncast<JSC::JSArray>(JSC::JSValue::decode(encodedValue).asCell());
static constexpr uint64_t noPresentIndex = std::numeric_limits<uint64_t>::max();

// Smallest index >= `start` whose slot in the array's vector storage holds a
// value, or noPresentIndex. Does not read an ArrayStorage array's sparse map.
static uint64_t nextPresentVectorIndex(JSC::JSArray* array, uint32_t start)
{
switch (array->indexingType()) {
case ALL_BLANK_INDEXING_TYPES:
case ALL_UNDECIDED_INDEXING_TYPES:
return notFound;
return noPresentIndex;

case ALL_INT32_INDEXING_TYPES:
case ALL_CONTIGUOUS_INDEXING_TYPES: {
Expand All @@ -7128,7 +7122,7 @@ extern "C" uint64_t Bun__JSArray__nextPresentIndex(
if (butterfly->contiguous().at(array, i))
return i;
}
return notFound;
return noPresentIndex;
}

case ALL_DOUBLE_INDEXING_TYPES: {
Expand All @@ -7142,7 +7136,7 @@ extern "C" uint64_t Bun__JSArray__nextPresentIndex(
if (value == value)
return i;
}
return notFound;
return noPresentIndex;
}

case ALL_ARRAY_STORAGE_INDEXING_TYPES: {
Expand All @@ -7152,15 +7146,7 @@ extern "C" uint64_t Bun__JSArray__nextPresentIndex(
if (storage->m_vector[i])
return i;
}

uint64_t result = notFound;
if (JSC::SparseArrayValueMap* map = storage->m_sparseMap.get()) {
for (const auto& entry : *map) {
if (entry.index() >= start && entry.index() < result)
result = entry.index();
}
}
return result;
return noPresentIndex;
}

default:
Expand All @@ -7169,6 +7155,80 @@ extern "C" uint64_t Bun__JSArray__nextPresentIndex(
}
}

static JSC::SparseArrayValueMap* sparseMapOf(JSC::JSArray* array)
{
if (!hasAnyArrayStorage(array->indexingType()))
return nullptr;
return array->butterfly()->arrayStorage()->m_sparseMap.get();
}

// Smallest own present index of a JSArray that is >= `start`, or UINT64_MAX
// when every index from `start` to the end of the array is a hole. Mirrors the
// butterfly walk in JSObject::getOwnIndexedPropertyNames so the caller can skip
// a run of holes without probing each index of a huge sparse array.
//
// Walks the whole sparse map on every call. A caller that asks once per run of
// holes with no bound on the runs uses Bun__JSArray__nextPresentVectorIndex and
// Bun__JSArray__copySortedSparseIndices instead.
extern "C" uint64_t Bun__JSArray__nextPresentIndex(
JSC::EncodedJSValue encodedValue,
uint32_t start)
{
JSC::JSArray* array = uncheckedDowncast<JSC::JSArray>(JSC::JSValue::decode(encodedValue).asCell());

uint64_t result = nextPresentVectorIndex(array, start);
if (result != noPresentIndex)
return result;

if (JSC::SparseArrayValueMap* map = sparseMapOf(array)) {
for (const auto& entry : *map) {
if (entry.index() >= start && entry.index() < result)
result = entry.index();
}
}
return result;
}

// The vector-storage half of Bun__JSArray__nextPresentIndex.
extern "C" uint64_t Bun__JSArray__nextPresentVectorIndex(
JSC::EncodedJSValue encodedValue,
uint32_t start)
{
JSC::JSArray* array = uncheckedDowncast<JSC::JSArray>(JSC::JSValue::decode(encodedValue).asCell());
return nextPresentVectorIndex(array, start);
}

// Copies the indices in [start, end) that the array's sparse map holds into
// `out`, ascending, the way JSObject::getOwnIndexedPropertyNames sorts them
// once. Returns how many there are. When that is more than `capacity`, `out`
// is incomplete and unsorted.
extern "C" uint32_t Bun__JSArray__copySortedSparseIndices(
Comment on lines +7191 to +7205

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 nit (optional): maintainers get two more hand-written exports in the monolithic bindings.cpp, which the repo's review rules ask new bindings to avoid. Bun__JSArray__nextPresentVectorIndex and Bun__JSArray__copySortedSparseIndices are new extern "C" entry points added at src/jsc/bindings/bindings.cpp:7191-7230 next to the pre-existing Bun__JSArray__nextPresentIndex. Fix: move the three JSArray storage helpers (nextPresentVectorIndex, sparseMapOf and the three exports) into their own file under src/jsc/bindings/ so the shared vector scan stays in one place without growing the god file; the Rust extern declarations in src/jsc/JSValue.rs need no change. [also at: src/jsc/bindings/bindings.cpp:7230 - nit: maintainers get two more exported bindings added to the monolithic bindings.cpp, which the repo's review rules ask new bindings to avoid.]

Why this was flagged

The diff adds two new extern "C" functions to src/jsc/bindings/bindings.cpp at lines 7191-7230 (Bun__JSArray__nextPresentVectorIndex and Bun__JSArray__copySortedSparseIndices) plus two static helpers. The repository review rules state 'no bindings in monolithic bindings.cpp' as an architecture rule. The PR description acknowledges this and says a move is mechanical. There is no runtime consequence; the base branch only had the one sibling export there. A fix is a file move; the Rust side links by symbol name so nothing else changes.

Verification: nit; acknowledged in diff: the PR description says "REVIEW.md asks for no new bindings there. If a new file is wanted, the move is mechanical" — the acknowledgement is accurate (the move is only a file move; the Rust side links by symbol name in src/jsc/JSValue.rs:2604 and :2620). Mechanism verified: git diff 29d9638..HEAD -- src/jsc/bindings/bindings.cpp adds two new extern "C" exports…

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The two new exports share one vector scan with Bun__JSArray__nextPresentIndex, which #33158 added to this file. A move has to take that function too, or the scan exists twice. The open #42247 edits that function, and the open #42238 adds its collector next to it, so a move now conflicts with both.

I left the placement as a question in the description. If a maintainer wants the new file, I make the move.

JSC::EncodedJSValue encodedValue,
uint32_t start,
uint32_t end,
uint32_t* out,
uint32_t capacity)
{
JSC::JSArray* array = uncheckedDowncast<JSC::JSArray>(JSC::JSValue::decode(encodedValue).asCell());

JSC::SparseArrayValueMap* map = sparseMapOf(array);
if (!map)
return 0;

uint32_t count = 0;
for (const auto& entry : *map) {
uint32_t index = entry.index();
if (index < start || index >= end)
continue;
if (count < capacity)
out[count] = index;
++count;
}
if (count <= capacity)
std::sort(out, out + count);
return count;
}

extern "C" void JSC__ArrayBuffer__ref(JSC::ArrayBuffer* self) { self->ref(); }
extern "C" void JSC__ArrayBuffer__deref(JSC::ArrayBuffer* self) { self->deref(); }
extern "C" void JSC__ArrayBuffer__asBunArrayBuffer(JSC::ArrayBuffer* self, Bun__ArrayBuffer* out)
Expand Down
6 changes: 2 additions & 4 deletions src/runtime/test_runner/diff_format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use bun_core::Output;
use bun_jsc::{JSGlobalObject, JSValue, JsResult};

use super::diff::print_diff::{print_diff_main, DiffConfig};
use super::pretty_format::{FormatOptions, JestPrettyFormat, MessageLevel};
use super::pretty_format::{FormatOptions, JestPrettyFormat};

/// Renders a Jest-style diff of two already-formatted values. Formatting a JS value runs user code
/// (getters, Proxy traps) and can throw, so it happens up front in [`DiffFormatter::new`], never
Expand All @@ -24,14 +24,13 @@ impl<'a> DiffFormatter<'a> {
not: bool,
) -> JsResult<DiffFormatter<'static>> {
let fmt_options = FormatOptions {
enable_colors: false,
add_newline: false,
flush: false,
quote_strings: true,
collapse_hole_runs: true,
};
let mut received_buf: Vec<u8> = Vec::new();
JestPrettyFormat::format(
MessageLevel::Debug,
global_this,
core::slice::from_ref(&received),
1,
Expand All @@ -40,7 +39,6 @@ impl<'a> DiffFormatter<'a> {
)?;
let mut expected_buf: Vec<u8> = Vec::new();
JestPrettyFormat::format(
MessageLevel::Debug,
global_this,
core::slice::from_ref(&expected),
1,
Expand Down
5 changes: 2 additions & 3 deletions src/runtime/test_runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,15 +220,14 @@ pub(crate) mod expect {
impl JSValueTestExt for JSValue {
#[inline]
fn jest_snapshot_pretty_format<W: bun_io::Write>(self, out: &mut W, global: &JSGlobalObject) -> JsResult<()> {
use super::pretty_format::{JestPrettyFormat, FormatOptions, MessageLevel};
use super::pretty_format::{JestPrettyFormat, FormatOptions};
let fmt_options = FormatOptions {
enable_colors: false,
add_newline: false,
flush: false,
quote_strings: true,
collapse_hole_runs: false,
};
JestPrettyFormat::format(
MessageLevel::Debug,
global,
core::slice::from_ref(&self),
1,
Expand Down
Loading
Loading