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

fix(ops): fallback when FastApiOneByteString is not utf8 #18518

Merged
merged 4 commits into from
Mar 31, 2023
Merged

fix(ops): fallback when FastApiOneByteString is not utf8 #18518

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
096bef7
fix(ops): fallback when FastApiOneByteString is not utf8
littledivy Mar 31, 2023
be81112
Merge branch 'main' of github.com:denoland/deno into fastops_utf8_val…
littledivy Mar 31, 2023
5e3a9ad
update tests
littledivy Mar 31, 2023
0af9c6d
Fix
littledivy Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 26 additions & 3 deletions ops/optimizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,13 +200,33 @@ impl Transform {
*ty = parse_quote! { *const #core::v8::fast_api::FastApiOneByteString };
match str_ty {
StringType::Ref => q!(Vars { var: &ident }, {
let var = unsafe { &*var }.as_str();
let var = match ::std::str::from_utf8(unsafe { &*var }.as_bytes()) {
Ok(v) => v,
Err(_) => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If memory serves, I had to do a separate default return value for pointer returning since there's no Default impl for a void pointer.

}
};
}),
StringType::Cow => q!(Vars { var: &ident }, {
let var = ::std::borrow::Cow::Borrowed(unsafe { &*var }.as_str());
let var = ::std::borrow::Cow::Borrowed(
match ::std::str::from_utf8(unsafe { &*var }.as_bytes()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to avoid doing the V8 fallback, and instead just create a String copy conditionally from the bytes?

Ok(v) => v,
Err(_) => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
},
);
}),
StringType::Owned => q!(Vars { var: &ident }, {
let var = unsafe { &*var }.as_str().to_owned();
let var = match ::std::str::from_utf8(unsafe { &*var }.as_bytes()) {
Ok(v) => v.to_owned(),
Err(_) => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
}),
}
}
Expand Down Expand Up @@ -718,6 +738,7 @@ impl Optimizer {
let segment = single_segment(segments)?;
match segment {
PathSegment { ident, .. } if ident == "str" => {
self.needs_fast_callback_option = true;
self.fast_parameters.push(FastValue::SeqOneByteString);
assert!(self
.transforms
Expand All @@ -742,6 +763,7 @@ impl Optimizer {
if let Some(val) = get_fast_scalar(ident.to_string().as_str()) {
self.fast_parameters.push(val);
} else if ident == "String" {
self.needs_fast_callback_option = true;
// Is `T` an owned String?
self.fast_parameters.push(FastValue::SeqOneByteString);
assert!(self
Expand Down Expand Up @@ -775,6 +797,7 @@ impl Optimizer {
}
// Is `T` a str?
PathSegment { ident, .. } if ident == "str" => {
self.needs_fast_callback_option = true;
self.fast_parameters.push(FastValue::SeqOneByteString);
assert!(self
.transforms
Expand Down
2 changes: 1 addition & 1 deletion ops/optimizer_tests/cow_str.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: false
has_ref_opstate: false
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: false
needs_fast_callback_option: true
fast_result: Some(Void)
fast_parameters: [V8Value, SeqOneByteString]
transforms: {0: Transform { kind: SeqOneByteString(Cow), index: 0 }}
Expand Down
13 changes: 11 additions & 2 deletions ops/optimizer_tests/cow_str.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl<'scope> deno_core::v8::fast_api::FastFunction for op_cow_str_fast {
fn args(&self) -> &'static [deno_core::v8::fast_api::Type] {
use deno_core::v8::fast_api::Type::*;
use deno_core::v8::fast_api::CType;
&[V8Value, SeqOneByteString]
&[V8Value, SeqOneByteString, CallbackOptions]
}
#[inline(always)]
fn return_type(&self) -> deno_core::v8::fast_api::CType {
Expand All @@ -82,10 +82,19 @@ impl<'scope> deno_core::v8::fast_api::FastFunction for op_cow_str_fast {
fn op_cow_str_fast_fn<'scope>(
_: deno_core::v8::Local<deno_core::v8::Object>,
c: *const deno_core::v8::fast_api::FastApiOneByteString,
fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions,
) -> () {
use deno_core::v8;
use deno_core::_ops;
let c = ::std::borrow::Cow::Borrowed(unsafe { &*c }.as_str());
let c = ::std::borrow::Cow::Borrowed(
match ::std::str::from_utf8(unsafe { &*c }.as_bytes()) {
Ok(v) => v,
Err(_) => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
},
);
let result = op_cow_str::call(c);
result
}
2 changes: 1 addition & 1 deletion ops/optimizer_tests/op_blob_revoke_object_url.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: true
has_ref_opstate: true
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: false
needs_fast_callback_option: true
fast_result: Some(Void)
fast_parameters: [V8Value, SeqOneByteString]
transforms: {1: Transform { kind: SeqOneByteString(Owned), index: 1 }}
Expand Down
2 changes: 1 addition & 1 deletion ops/optimizer_tests/op_print.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: true
has_ref_opstate: true
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: false
needs_fast_callback_option: true
fast_result: Some(Void)
fast_parameters: [V8Value, SeqOneByteString, Bool]
transforms: {1: Transform { kind: SeqOneByteString(Ref), index: 1 }}
Expand Down
2 changes: 1 addition & 1 deletion ops/optimizer_tests/owned_string.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: false
has_ref_opstate: false
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: false
needs_fast_callback_option: true
fast_result: Some(U32)
fast_parameters: [V8Value, SeqOneByteString]
transforms: {0: Transform { kind: SeqOneByteString(Owned), index: 0 }}
Expand Down
11 changes: 9 additions & 2 deletions ops/optimizer_tests/owned_string.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ impl<'scope> deno_core::v8::fast_api::FastFunction for op_string_length_fast {
fn args(&self) -> &'static [deno_core::v8::fast_api::Type] {
use deno_core::v8::fast_api::Type::*;
use deno_core::v8::fast_api::CType;
&[V8Value, SeqOneByteString]
&[V8Value, SeqOneByteString, CallbackOptions]
}
#[inline(always)]
fn return_type(&self) -> deno_core::v8::fast_api::CType {
Expand All @@ -94,10 +94,17 @@ impl<'scope> deno_core::v8::fast_api::FastFunction for op_string_length_fast {
fn op_string_length_fast_fn<'scope>(
_: deno_core::v8::Local<deno_core::v8::Object>,
string: *const deno_core::v8::fast_api::FastApiOneByteString,
fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions,
) -> u32 {
use deno_core::v8;
use deno_core::_ops;
let string = unsafe { &*string }.as_str().to_owned();
let string = match ::std::str::from_utf8(unsafe { &*string }.as_bytes()) {
Ok(v) => v.to_owned(),
Err(_) => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
let result = op_string_length::call(string);
result
}
2 changes: 1 addition & 1 deletion ops/optimizer_tests/strings.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: false
has_ref_opstate: false
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: false
needs_fast_callback_option: true
fast_result: Some(U32)
fast_parameters: [V8Value, SeqOneByteString]
transforms: {0: Transform { kind: SeqOneByteString(Ref), index: 0 }}
Expand Down
11 changes: 9 additions & 2 deletions ops/optimizer_tests/strings.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ impl<'scope> deno_core::v8::fast_api::FastFunction for op_string_length_fast {
fn args(&self) -> &'static [deno_core::v8::fast_api::Type] {
use deno_core::v8::fast_api::Type::*;
use deno_core::v8::fast_api::CType;
&[V8Value, SeqOneByteString]
&[V8Value, SeqOneByteString, CallbackOptions]
}
#[inline(always)]
fn return_type(&self) -> deno_core::v8::fast_api::CType {
Expand All @@ -95,10 +95,17 @@ impl<'scope> deno_core::v8::fast_api::FastFunction for op_string_length_fast {
fn op_string_length_fast_fn<'scope>(
_: deno_core::v8::Local<deno_core::v8::Object>,
string: *const deno_core::v8::fast_api::FastApiOneByteString,
fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions,
) -> u32 {
use deno_core::v8;
use deno_core::_ops;
let string = unsafe { &*string }.as_str();
let string = match ::std::str::from_utf8(unsafe { &*string }.as_bytes()) {
Ok(v) => v,
Err(_) => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
let result = op_string_length::call(string);
result
}
2 changes: 1 addition & 1 deletion ops/optimizer_tests/strings_result.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: true
has_ref_opstate: false
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: false
needs_fast_callback_option: true
fast_result: Some(U32)
fast_parameters: [V8Value, SeqOneByteString]
transforms: {0: Transform { kind: SeqOneByteString(Ref), index: 0 }}
Expand Down