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.

Sign up for GitHub

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

Jump to bottom

src: register external references for source code #47055

Merged
merged 2 commits into from
May 10, 2023
Merged

src: register external references for source code #47055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0bd46c
src: register external references for source code
kvakil Mar 9, 2023
cd42560
fixup! src: register external references for source code
kvakil Apr 9, 2023
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
46 changes: 27 additions & 19 deletions src/node_builtins.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,18 +213,18 @@ MaybeLocal<String> BuiltinLoader::LoadBuiltinSource(Isolate* isolate,

namespace {
static Mutex externalized_builtins_mutex;
std::unordered_map<std::string, std::string> externalized_builtin_sources;
std::unordered_map<std::string, std::unique_ptr<StaticExternalTwoByteResource>>
externalized_builtin_sources;
} // namespace

void BuiltinLoader::AddExternalizedBuiltin(const char* id,
const char* filename) {
std::string source;
StaticExternalTwoByteResource* resource;
{
Mutex::ScopedLock lock(externalized_builtins_mutex);
auto it = externalized_builtin_sources.find(id);
if (it != externalized_builtin_sources.end()) {
source = it->second;
} else {
if (it == externalized_builtin_sources.end()) {
std::string source;
int r = ReadFileSync(&source, filename);
if (r != 0) {
fprintf(stderr,
Expand All @@ -233,23 +233,29 @@ void BuiltinLoader::AddExternalizedBuiltin(const char* id,
filename);
ABORT();
}
externalized_builtin_sources[id] = source;
size_t expected_u16_length =
simdutf::utf16_length_from_utf8(source.data(), source.length());
auto out = std::make_shared<std::vector<uint16_t>>(expected_u16_length);
size_t u16_length = simdutf::convert_utf8_to_utf16(
source.data(),
source.length(),
reinterpret_cast<char16_t*>(out->data()));
out->resize(u16_length);

auto result = externalized_builtin_sources.emplace(
id,
std::make_unique<StaticExternalTwoByteResource>(
out->data(), out->size(), out));
CHECK(result.second);
it = result.first;
}
// OK to get the raw pointer, since externalized_builtin_sources owns
Copy link
Contributor

Choose a reason for hiding this comment

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

@kvakil Are you sure that v8::String::ExternalStringResourceBase doesn't expect to own the allocation? I have another PR that is affected by this and in my case v8::String::ExternalStringResourceBase::Dispose calls free on the received string when destroying the isolate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmomtchev this is V8's documentation/default implementation of v8::String::ExternalStringResourceBase::Dispose:

/**
 * Internally V8 will call this Dispose method when the external string
 * resource is no longer needed. The default implementation will use the
 * delete operator. This method can be overridden in subclasses to
 * control how allocated external string resources are disposed.
 */
virtual void Dispose() { delete this; }

Since this diff has StaticExternalByteResource override Dispose to be empty, we never execute delete this:

void Dispose() override {
  // We ignore Dispose calls from V8, even if we "own" a resource via
  // owning_ptr_. All instantiations of this class are static or owned by a
  // static map, and will be destructed when static variables are destructed.
}

so I believe this is correct.

// the resource, resources are never removed from the map, and
// externalized_builtin_sources has static lifetime.
resource = it->second.get();
}

Add(id, source);
}

bool BuiltinLoader::Add(const char* id, std::string_view utf8source) {
legendecas marked this conversation as resolved.
Show resolved Hide resolved
size_t expected_u16_length =
simdutf::utf16_length_from_utf8(utf8source.data(), utf8source.length());
auto out = std::make_shared<std::vector<uint16_t>>(expected_u16_length);
size_t u16_length =
simdutf::convert_utf8_to_utf16(utf8source.data(),
utf8source.length(),
reinterpret_cast<char16_t*>(out->data()));
out->resize(u16_length);
return Add(id, UnionBytes(out));
Add(id, UnionBytes(resource));
}

MaybeLocal<Function> BuiltinLoader::LookupAndCompileInternal(
Expand Down Expand Up @@ -715,6 +721,8 @@ void BuiltinLoader::RegisterExternalReferences(
registry->Register(CompileFunction);
registry->Register(HasCachedBuiltins);
registry->Register(SetInternalLoaders);

RegisterExternalReferencesForInternalizedBuiltinCode(registry);
}

} // namespace builtins
Expand Down
6 changes: 5 additions & 1 deletion src/node_builtins.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <set>
#include <string>
#include <vector>
#include "node_external_reference.h"
#include "node_mutex.h"
#include "node_threadsafe_cow.h"
#include "node_union_bytes.h"
Expand All @@ -30,6 +31,10 @@ using BuiltinCodeCacheMap =
std::unordered_map<std::string,
std::unique_ptr<v8::ScriptCompiler::CachedData>>;

// Generated by tools/js2c.py as node_javascript.cc
void RegisterExternalReferencesForInternalizedBuiltinCode(
ExternalReferenceRegistry* registry);

struct CodeCacheInfo {
std::string id;
std::vector<uint8_t> data;
Expand Down Expand Up @@ -72,7 +77,6 @@ class NODE_EXTERN_PRIVATE BuiltinLoader {
v8::Local<v8::String> GetConfigString(v8::Isolate* isolate);
bool Exists(const char* id);
bool Add(const char* id, const UnionBytes& source);
bool Add(const char* id, std::string_view utf8source);

bool CompileAllBuiltins(v8::Local<v8::Context> context);
void RefreshCodeCache(const std::vector<CodeCacheInfo>& in);
Expand Down
3 changes: 2 additions & 1 deletion src/node_external_reference.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ class ExternalReferenceRegistry {
V(v8::IndexedPropertyDefinerCallback) \
V(v8::IndexedPropertyDeleterCallback) \
V(v8::IndexedPropertyQueryCallback) \
V(v8::IndexedPropertyDescriptorCallback)
V(v8::IndexedPropertyDescriptorCallback) \
V(const v8::String::ExternalStringResourceBase*)

#define V(ExternalReferenceType) \
void Register(ExternalReferenceType addr) { RegisterT(addr); }
Expand Down
76 changes: 50 additions & 26 deletions src/node_union_bytes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,50 +4,74 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

// A union of const uint8_t* or const uint16_t* data that can be
// turned into external v8::String when given an isolate.

#include "v8.h"

namespace node {

// An external resource intended to be used with static lifetime.
template <typename Char, typename IChar, typename Base>
class StaticExternalByteResource : public Base {
static_assert(sizeof(IChar) == sizeof(Char),
"incompatible interface and internal pointers");

public:
explicit StaticExternalByteResource(const Char* data,
size_t length,
std::shared_ptr<void> owning_ptr)
: data_(data), length_(length), owning_ptr_(owning_ptr) {}

const IChar* data() const override {
return reinterpret_cast<const IChar*>(data_);
}
size_t length() const override { return length_; }

void Dispose() override {
// We ignore Dispose calls from V8, even if we "own" a resource via
// owning_ptr_. All instantiations of this class are static or owned by a
// static map, and will be destructed when static variables are destructed.
}

StaticExternalByteResource(const StaticExternalByteResource&) = delete;
StaticExternalByteResource& operator=(const StaticExternalByteResource&) =
delete;

private:
const Char* data_;
const size_t length_;
std::shared_ptr<void> owning_ptr_;
};

using StaticExternalOneByteResource =
StaticExternalByteResource<uint8_t,
char,
v8::String::ExternalOneByteStringResource>;
using StaticExternalTwoByteResource =
StaticExternalByteResource<uint16_t,
uint16_t,
v8::String::ExternalStringResource>;

// Similar to a v8::String, but it's independent from Isolates
// and can be materialized in Isolates as external Strings
// via ToStringChecked.
class UnionBytes {
public:
UnionBytes(const uint16_t* data, size_t length)
: one_bytes_(nullptr), two_bytes_(data), length_(length) {}
UnionBytes(const uint8_t* data, size_t length)
: one_bytes_(data), two_bytes_(nullptr), length_(length) {}
template <typename T> // T = uint8_t or uint16_t
explicit UnionBytes(std::shared_ptr<std::vector</*const*/ T>> data)
: UnionBytes(data->data(), data->size()) {
owning_ptr_ = data;
}
explicit UnionBytes(StaticExternalOneByteResource* one_byte_resource)
: one_byte_resource_(one_byte_resource), two_byte_resource_(nullptr) {}
explicit UnionBytes(StaticExternalTwoByteResource* two_byte_resource)
: one_byte_resource_(nullptr), two_byte_resource_(two_byte_resource) {}

UnionBytes(const UnionBytes&) = default;
UnionBytes& operator=(const UnionBytes&) = default;
UnionBytes(UnionBytes&&) = default;
UnionBytes& operator=(UnionBytes&&) = default;

bool is_one_byte() const { return one_bytes_ != nullptr; }
const uint16_t* two_bytes_data() const {
CHECK_NOT_NULL(two_bytes_);
return two_bytes_;
}
const uint8_t* one_bytes_data() const {
CHECK_NOT_NULL(one_bytes_);
return one_bytes_;
}
bool is_one_byte() const { return one_byte_resource_ != nullptr; }

v8::Local<v8::String> ToStringChecked(v8::Isolate* isolate) const;
size_t length() const { return length_; }

private:
const uint8_t* one_bytes_;
const uint16_t* two_bytes_;
size_t length_;
std::shared_ptr<void> owning_ptr_;
StaticExternalOneByteResource* one_byte_resource_;
StaticExternalTwoByteResource* two_byte_resource_;
};

} // namespace node
Expand Down
60 changes: 4 additions & 56 deletions src/util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,65 +551,13 @@ void SetConstructorFunction(Isolate* isolate,
that->Set(name, tmpl);
}

namespace {

class NonOwningExternalOneByteResource
: public v8::String::ExternalOneByteStringResource {
public:
explicit NonOwningExternalOneByteResource(const UnionBytes& source)
: source_(source) {}
~NonOwningExternalOneByteResource() override = default;

const char* data() const override {
return reinterpret_cast<const char*>(source_.one_bytes_data());
}
size_t length() const override { return source_.length(); }

NonOwningExternalOneByteResource(const NonOwningExternalOneByteResource&) =
delete;
NonOwningExternalOneByteResource& operator=(
const NonOwningExternalOneByteResource&) = delete;

private:
const UnionBytes source_;
};

class NonOwningExternalTwoByteResource
: public v8::String::ExternalStringResource {
public:
explicit NonOwningExternalTwoByteResource(const UnionBytes& source)
: source_(source) {}
~NonOwningExternalTwoByteResource() override = default;

const uint16_t* data() const override { return source_.two_bytes_data(); }
size_t length() const override { return source_.length(); }

NonOwningExternalTwoByteResource(const NonOwningExternalTwoByteResource&) =
delete;
NonOwningExternalTwoByteResource& operator=(
const NonOwningExternalTwoByteResource&) = delete;

private:
const UnionBytes source_;
};

} // anonymous namespace

Local<String> UnionBytes::ToStringChecked(Isolate* isolate) const {
if (UNLIKELY(length() == 0)) {
// V8 requires non-null data pointers for empty external strings,
// but we don't guarantee that. Solve this by not creating an
// external string at all in that case.
return String::Empty(isolate);
}
if (is_one_byte()) {
NonOwningExternalOneByteResource* source =
new NonOwningExternalOneByteResource(*this);
return String::NewExternalOneByte(isolate, source).ToLocalChecked();
return String::NewExternalOneByte(isolate, one_byte_resource_)
.ToLocalChecked();
} else {
NonOwningExternalTwoByteResource* source =
new NonOwningExternalTwoByteResource(*this);
return String::NewExternalTwoByte(isolate, source).ToLocalChecked();
return String::NewExternalTwoByte(isolate, two_byte_resource_)
.ToLocalChecked();
}
}

Expand Down
Loading