From 76f689cd0626540d8db5d12b4d827d89748a9ab7 Mon Sep 17 00:00:00 2001 From: David Raifaizen Date: Wed, 6 Jan 2016 19:41:52 -0500 Subject: [PATCH] Changed blob to use DataSlice with Arc in order to limit wasteful copying of byte vector --- components/script/dom/blob.rs | 222 +++++++++++++++------------ components/script/dom/file.rs | 3 +- components/script/dom/filereader.rs | 43 ++---- components/script/dom/testbinding.rs | 8 +- components/script/dom/websocket.rs | 9 +- tests/unit/script/dom/blob.rs | 34 ++++ tests/unit/script/lib.rs | 3 + 7 files changed, 190 insertions(+), 132 deletions(-) create mode 100644 tests/unit/script/dom/blob.rs diff --git a/components/script/dom/blob.rs b/components/script/dom/blob.rs index d47fbde3f25..88202081bf3 100644 --- a/components/script/dom/blob.rs +++ b/components/script/dom/blob.rs @@ -8,104 +8,30 @@ use dom::bindings::error::Fallible; use dom::bindings::global::{GlobalField, GlobalRef}; use dom::bindings::js::Root; use dom::bindings::reflector::{Reflector, reflect_dom_object}; +use dom::bindings::trace::JSTraceable; use num::ToPrimitive; use std::ascii::AsciiExt; use std::borrow::ToOwned; use std::cell::Cell; use std::cmp::{max, min}; -use std::sync::mpsc::Sender; +use std::sync::Arc; use util::str::DOMString; -// https://w3c.github.io/FileAPI/#blob -#[dom_struct] -pub struct Blob { - reflector_: Reflector, - bytes: Option>, - typeString: String, - global: GlobalField, - isClosed_: Cell, +#[derive(Clone, JSTraceable)] +pub struct DataSlice { + bytes: Arc>, + bytes_start: usize, + bytes_end: usize } -fn is_ascii_printable(string: &str) -> bool { - // Step 5.1 in Sec 5.1 of File API spec - // https://w3c.github.io/FileAPI/#constructorBlob - string.chars().all(|c| c >= '\x20' && c <= '\x7E') -} - -impl Blob { - pub fn new_inherited(global: GlobalRef, bytes: Option>, typeString: &str) -> Blob { - Blob { - reflector_: Reflector::new(), - bytes: bytes, - typeString: typeString.to_owned(), - global: GlobalField::from_rooted(&global), - isClosed_: Cell::new(false), - } - } - - pub fn new(global: GlobalRef, bytes: Option>, typeString: &str) -> Root { - reflect_dom_object(box Blob::new_inherited(global, bytes, typeString), - global, - BlobBinding::Wrap) - } - - // https://w3c.github.io/FileAPI/#constructorBlob - pub fn Constructor(global: GlobalRef) -> Fallible> { - Ok(Blob::new(global, None, "")) - } - - // https://w3c.github.io/FileAPI/#constructorBlob - pub fn Constructor_(global: GlobalRef, - blobParts: DOMString, - blobPropertyBag: &BlobBinding::BlobPropertyBag) - -> Fallible> { - // TODO: accept other blobParts types - ArrayBuffer or ArrayBufferView or Blob - // FIXME(ajeffrey): convert directly from a DOMString to a Vec - let bytes: Option> = Some(String::from(blobParts).into_bytes()); - let typeString = if is_ascii_printable(&blobPropertyBag.type_) { - &*blobPropertyBag.type_ - } else { - "" - }; - Ok(Blob::new(global, bytes, &typeString.to_ascii_lowercase())) - } - - pub fn read_out_buffer(&self, send: Sender>) { - send.send(self.bytes.clone().unwrap_or(vec![])).unwrap(); - } - - // simpler to use version of read_out_buffer - pub fn clone_bytes(&self) -> Vec { - self.bytes.clone().unwrap_or(vec![]) - } -} - -impl BlobMethods for Blob { - // https://w3c.github.io/FileAPI/#dfn-size - fn Size(&self) -> u64 { - match self.bytes { - None => 0, - Some(ref bytes) => bytes.len() as u64, - } - } - - // https://w3c.github.io/FileAPI/#dfn-type - fn Type(&self) -> DOMString { - DOMString::from(self.typeString.clone()) - } - - // https://w3c.github.io/FileAPI/#slice-method-algo - fn Slice(&self, - start: Option, - end: Option, - contentType: Option) - -> Root { - let size: i64 = self.Size().to_i64().unwrap(); +impl DataSlice { + pub fn new(bytes: Arc>, start: Option, end: Option) -> DataSlice { + let size = bytes.len() as i64; let relativeStart: i64 = match start { None => 0, Some(start) => { if start < 0 { - max(size.to_i64().unwrap() + start, 0) + max(size + start, 0) } else { min(start, size) } @@ -121,6 +47,119 @@ impl BlobMethods for Blob { } } }; + + let span: i64 = max(relativeEnd - relativeStart, 0); + let start = relativeStart.to_usize().unwrap(); + let end = (relativeStart + span).to_usize().unwrap(); + + DataSlice { + bytes: bytes, + bytes_start: start, + bytes_end: end + } + } + + pub fn get_bytes(&self) -> &[u8] { + &self.bytes[self.bytes_start..self.bytes_end] + } + + pub fn size(&self) -> u64 { + (self.bytes_end as u64) - (self.bytes_start as u64) + } +} + + +// https://w3c.github.io/FileAPI/#blob +#[dom_struct] +pub struct Blob { + reflector_: Reflector, + #[ignore_heap_size_of = "No clear owner"] + data: DataSlice, + typeString: String, + global: GlobalField, + isClosed_: Cell, +} + +fn is_ascii_printable(string: &str) -> bool { + // Step 5.1 in Sec 5.1 of File API spec + // https://w3c.github.io/FileAPI/#constructorBlob + string.chars().all(|c| c >= '\x20' && c <= '\x7E') +} + +impl Blob { + pub fn new_inherited(global: GlobalRef, + bytes: Arc>, + bytes_start: Option, + bytes_end: Option, + typeString: &str) -> Blob { + Blob { + reflector_: Reflector::new(), + data: DataSlice::new(bytes, bytes_start, bytes_end), + typeString: typeString.to_owned(), + global: GlobalField::from_rooted(&global), + isClosed_: Cell::new(false), + } + } + + pub fn new(global: GlobalRef, bytes: Vec, typeString: &str) -> Root { + let boxed_blob = box Blob::new_inherited(global, Arc::new(bytes), None, None, typeString); + reflect_dom_object(boxed_blob, global, BlobBinding::Wrap) + } + + fn new_sliced(global: GlobalRef, + bytes: Arc>, + bytes_start: Option, + bytes_end: Option, + typeString: &str) -> Root { + + let boxed_blob = box Blob::new_inherited(global, bytes, bytes_start, bytes_end, typeString); + reflect_dom_object(boxed_blob, global, BlobBinding::Wrap) + } + + // https://w3c.github.io/FileAPI/#constructorBlob + pub fn Constructor(global: GlobalRef) -> Fallible> { + Ok(Blob::new(global, Vec::new(), "")) + } + + // https://w3c.github.io/FileAPI/#constructorBlob + pub fn Constructor_(global: GlobalRef, + blobParts: DOMString, + blobPropertyBag: &BlobBinding::BlobPropertyBag) + -> Fallible> { + // TODO: accept other blobParts types - ArrayBuffer or ArrayBufferView or Blob + // FIXME(ajeffrey): convert directly from a DOMString to a Vec + let bytes: Vec = String::from(blobParts).into_bytes(); + let typeString = if is_ascii_printable(&blobPropertyBag.type_) { + &*blobPropertyBag.type_ + } else { + "" + }; + Ok(Blob::new(global, bytes, &typeString.to_ascii_lowercase())) + } + + pub fn get_data(&self) -> &DataSlice { + &self.data + } +} + +impl BlobMethods for Blob { + // https://w3c.github.io/FileAPI/#dfn-size + fn Size(&self) -> u64 { + self.data.size() + } + + // https://w3c.github.io/FileAPI/#dfn-type + fn Type(&self) -> DOMString { + DOMString::from(self.typeString.clone()) + } + + // https://w3c.github.io/FileAPI/#slice-method-algo + fn Slice(&self, + start: Option, + end: Option, + contentType: Option) + -> Root { + let relativeContentType = match contentType { None => DOMString::new(), Some(mut str) => { @@ -132,18 +171,9 @@ impl BlobMethods for Blob { } } }; - let span: i64 = max(relativeEnd - relativeStart, 0); let global = self.global.root(); - match self.bytes { - None => Blob::new(global.r(), None, &relativeContentType), - Some(ref vec) => { - let start = relativeStart.to_usize().unwrap(); - let end = (relativeStart + span).to_usize().unwrap(); - let mut bytes: Vec = Vec::new(); - bytes.extend_from_slice(&vec[start..end]); - Blob::new(global.r(), Some(bytes), &relativeContentType) - } - } + let bytes = self.data.bytes.clone(); + Blob::new_sliced(global.r(), bytes, start, end, &relativeContentType) } // https://w3c.github.io/FileAPI/#dfn-isClosed diff --git a/components/script/dom/file.rs b/components/script/dom/file.rs index 5dfffd62541..bdf06afb54a 100644 --- a/components/script/dom/file.rs +++ b/components/script/dom/file.rs @@ -8,6 +8,7 @@ use dom::bindings::global::GlobalRef; use dom::bindings::js::Root; use dom::bindings::reflector::reflect_dom_object; use dom::blob::Blob; +use std::sync::Arc; use util::str::DOMString; #[dom_struct] @@ -21,7 +22,7 @@ impl File { _file_bits: &Blob, name: DOMString) -> File { File { //TODO: get type from the underlying filesystem instead of "".to_string() - blob: Blob::new_inherited(global, None, ""), + blob: Blob::new_inherited(global, Arc::new(Vec::new()), None, None, ""), name: name, } // XXXManishearth Once Blob is able to store data diff --git a/components/script/dom/filereader.rs b/components/script/dom/filereader.rs index 1627fde901a..640cdca19a5 100644 --- a/components/script/dom/filereader.rs +++ b/components/script/dom/filereader.rs @@ -12,7 +12,7 @@ use dom::bindings::inheritance::Castable; use dom::bindings::js::{JS, MutNullableHeap, Root}; use dom::bindings::refcounted::Trusted; use dom::bindings::reflector::{Reflectable, reflect_dom_object}; -use dom::blob::Blob; +use dom::blob::{Blob, DataSlice}; use dom::domexception::{DOMErrorName, DOMException}; use dom::event::{Event, EventBubbles, EventCancelable}; use dom::eventtarget::EventTarget; @@ -23,10 +23,8 @@ use encoding::types::{DecoderTrap, EncodingRef}; use hyper::mime::{Attr, Mime}; use rustc_serialize::base64::{CharacterSet, Config, Newline, ToBase64}; use script_task::ScriptTaskEventCategory::FileRead; -use script_task::{CommonScriptMsg, Runnable, ScriptChan, ScriptPort}; +use script_task::{CommonScriptMsg, Runnable, ScriptChan}; use std::cell::Cell; -use std::sync::mpsc; -use std::sync::mpsc::Receiver; use string_cache::Atom; use util::str::DOMString; use util::task::spawn_named; @@ -163,7 +161,7 @@ impl FileReader { // https://w3c.github.io/FileAPI/#dfn-readAsText pub fn process_read_eof(filereader: TrustedFileReader, gen_id: GenerationId, - data: ReadMetaData, blob_contents: Vec) { + data: ReadMetaData, blob_contents: DataSlice) { let fr = filereader.root(); macro_rules! return_on_abort( @@ -178,11 +176,13 @@ impl FileReader { // Step 8.1 fr.change_ready_state(FileReaderReadyState::Done); // Step 8.2 + + let bytes = blob_contents.get_bytes(); let output = match data.function { FileReaderFunction::ReadAsDataUrl => - FileReader::perform_readasdataurl(data, blob_contents), + FileReader::perform_readasdataurl(data, bytes), FileReaderFunction::ReadAsText => - FileReader::perform_readastext(data, blob_contents), + FileReader::perform_readastext(data, bytes), }; *fr.result.borrow_mut() = Some(output); @@ -200,12 +200,11 @@ impl FileReader { } // https://w3c.github.io/FileAPI/#dfn-readAsText - fn perform_readastext(data: ReadMetaData, blob_contents: Vec) + fn perform_readastext(data: ReadMetaData, blob_bytes: &[u8]) -> DOMString { let blob_label = &data.label; let blob_type = &data.blobtype; - let blob_bytes = &blob_contents[..]; //https://w3c.github.io/FileAPI/#encoding-determination // Steps 1 & 2 & 3 @@ -233,7 +232,7 @@ impl FileReader { } //https://w3c.github.io/FileAPI/#dfn-readAsDataURL - fn perform_readasdataurl(data: ReadMetaData, blob_contents: Vec) + fn perform_readasdataurl(data: ReadMetaData, bytes: &[u8]) -> DOMString { let config = Config { char_set: CharacterSet::UrlSafe, @@ -241,7 +240,7 @@ impl FileReader { pad: true, line_length: None }; - let base64 = blob_contents.to_base64(config); + let base64 = bytes.to_base64(config); let output = if data.blobtype.is_empty() { format!("data:base64,{}", base64) @@ -355,8 +354,8 @@ impl FileReader { self.change_ready_state(FileReaderReadyState::Loading); // Step 4 - let (send, bytes) = mpsc::channel(); - blob.read_out_buffer(send); + let blob_contents = blob.get_data().clone(); + let type_ = blob.Type(); let load_data = ReadMetaData::new(String::from(type_), label.map(String::from), function); @@ -367,7 +366,7 @@ impl FileReader { let script_chan = global.file_reading_task_source(); spawn_named("file reader async operation".to_owned(), move || { - perform_annotated_read_operation(gen_id, load_data, bytes, fr, script_chan) + perform_annotated_read_operation(gen_id, load_data, blob_contents, fr, script_chan) }); Ok(()) } @@ -382,7 +381,7 @@ pub enum FileReaderEvent { ProcessRead(TrustedFileReader, GenerationId), ProcessReadData(TrustedFileReader, GenerationId), ProcessReadError(TrustedFileReader, GenerationId, DOMErrorName), - ProcessReadEOF(TrustedFileReader, GenerationId, ReadMetaData, Vec) + ProcessReadEOF(TrustedFileReader, GenerationId, ReadMetaData, DataSlice) } impl Runnable for FileReaderEvent { @@ -406,7 +405,7 @@ impl Runnable for FileReaderEvent { } // https://w3c.github.io/FileAPI/#task-read-operation -fn perform_annotated_read_operation(gen_id: GenerationId, data: ReadMetaData, blob_contents: Receiver>, +fn perform_annotated_read_operation(gen_id: GenerationId, data: ReadMetaData, blob_contents: DataSlice, filereader: TrustedFileReader, script_chan: Box) { let chan = &script_chan; // Step 4 @@ -416,16 +415,6 @@ fn perform_annotated_read_operation(gen_id: GenerationId, data: ReadMetaData, bl let task = box FileReaderEvent::ProcessReadData(filereader.clone(), gen_id); chan.send(CommonScriptMsg::RunnableMsg(FileRead, task)).unwrap(); - let bytes = match blob_contents.recv() { - Ok(bytes) => bytes, - Err(_) => { - let task = box FileReaderEvent::ProcessReadError(filereader, - gen_id, DOMErrorName::NotFoundError); - chan.send(CommonScriptMsg::RunnableMsg(FileRead, task)).unwrap(); - return; - } - }; - - let task = box FileReaderEvent::ProcessReadEOF(filereader, gen_id, data, bytes); + let task = box FileReaderEvent::ProcessReadEOF(filereader, gen_id, data, blob_contents); chan.send(CommonScriptMsg::RunnableMsg(FileRead, task)).unwrap(); } diff --git a/components/script/dom/testbinding.rs b/components/script/dom/testbinding.rs index 1eed6e8eb5f..bc969d69ffd 100644 --- a/components/script/dom/testbinding.rs +++ b/components/script/dom/testbinding.rs @@ -85,7 +85,7 @@ impl TestBindingMethods for TestBinding { fn EnumAttribute(&self) -> TestEnum { TestEnum::_empty } fn SetEnumAttribute(&self, _: TestEnum) {} fn InterfaceAttribute(&self) -> Root { - Blob::new(global_root_from_reflector(self).r(), None, "") + Blob::new(global_root_from_reflector(self).r(), Vec::new(), "") } fn SetInterfaceAttribute(&self, _: &Blob) {} fn UnionAttribute(&self) -> HTMLElementOrLong { HTMLElementOrLong::eLong(0) } @@ -143,7 +143,7 @@ impl TestBindingMethods for TestBinding { fn SetAttr_to_automatically_rename(&self, _: DOMString) {} fn GetEnumAttributeNullable(&self) -> Option { Some(TestEnum::_empty) } fn GetInterfaceAttributeNullable(&self) -> Option> { - Some(Blob::new(global_root_from_reflector(self).r(), None, "")) + Some(Blob::new(global_root_from_reflector(self).r(), Vec::new(), "")) } fn SetInterfaceAttributeNullable(&self, _: Option<&Blob>) {} fn GetInterfaceAttributeWeak(&self) -> Option> { @@ -182,7 +182,7 @@ impl TestBindingMethods for TestBinding { fn ReceiveByteString(&self) -> ByteString { ByteString::new(vec!()) } fn ReceiveEnum(&self) -> TestEnum { TestEnum::_empty } fn ReceiveInterface(&self) -> Root { - Blob::new(global_root_from_reflector(self).r(), None, "") + Blob::new(global_root_from_reflector(self).r(), Vec::new(), "") } fn ReceiveAny(&self, _: *mut JSContext) -> JSVal { NullValue() } fn ReceiveObject(&self, _: *mut JSContext) -> *mut JSObject { panic!() } @@ -207,7 +207,7 @@ impl TestBindingMethods for TestBinding { fn ReceiveNullableByteString(&self) -> Option { Some(ByteString::new(vec!())) } fn ReceiveNullableEnum(&self) -> Option { Some(TestEnum::_empty) } fn ReceiveNullableInterface(&self) -> Option> { - Some(Blob::new(global_root_from_reflector(self).r(), None, "")) + Some(Blob::new(global_root_from_reflector(self).r(), Vec::new(), "")) } fn ReceiveNullableObject(&self, _: *mut JSContext) -> *mut JSObject { ptr::null_mut() } fn ReceiveNullableUnion(&self) -> Option { diff --git a/components/script/dom/websocket.rs b/components/script/dom/websocket.rs index 35e3095293f..73c488326c5 100644 --- a/components/script/dom/websocket.rs +++ b/components/script/dom/websocket.rs @@ -381,19 +381,20 @@ impl WebSocketMethods for WebSocket { } // https://html.spec.whatwg.org/multipage/#dom-websocket-send - fn Send_(&self, data: &Blob) -> Fallible<()> { + fn Send_(&self, blob: &Blob) -> Fallible<()> { /* As per https://html.spec.whatwg.org/multipage/#websocket the buffered amount needs to be clamped to u32, even though Blob.Size() is u64 If the buffer limit is reached in the first place, there are likely other major problems */ - let data_byte_len = data.Size(); + let data_byte_len = blob.Size(); let send_data = try!(self.send_impl(data_byte_len)); if send_data { let mut other_sender = self.sender.borrow_mut(); let my_sender = other_sender.as_mut().unwrap(); - let _ = my_sender.send(WebSocketDomAction::SendMessage(MessageData::Binary(data.clone_bytes()))); + let bytes = blob.get_data().get_bytes().to_vec(); + let _ = my_sender.send(WebSocketDomAction::SendMessage(MessageData::Binary(bytes))); } Ok(()) @@ -579,7 +580,7 @@ impl Runnable for MessageReceivedTask { MessageData::Binary(data) => { match ws.binary_type.get() { BinaryType::Blob => { - let blob = Blob::new(global.r(), Some(data), ""); + let blob = Blob::new(global.r(), data, ""); blob.to_jsval(cx, message.handle_mut()); } BinaryType::Arraybuffer => { diff --git a/tests/unit/script/dom/blob.rs b/tests/unit/script/dom/blob.rs new file mode 100644 index 00000000000..339b1f4ba36 --- /dev/null +++ b/tests/unit/script/dom/blob.rs @@ -0,0 +1,34 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use script::dom::blob::{DataSlice}; +use std::sync::Arc; + +#[test] +fn test_data_slice_without_start_end_should_match_buffer_size() { + let bytes = Arc::new(vec![1u8, 2u8, 3u8]); + let data = DataSlice::new(bytes, None, None); + assert_eq!(data.size(), 3); +} + +#[test] +fn test_data_slice_should_prevent_reverse_bounds() { + let bytes = Arc::new(vec![1u8, 2, 3, 4, 5]); + let start = Some(3); + let end = Some(1); + + let data = DataSlice::new(bytes, start, end); + assert_eq!(data.size(), 0); +} + +#[test] +fn test_data_slice_should_respect_correct_bounds() { + let bytes = Arc::new(vec![1u8, 2, 3, 4, 5]); + let start = Some(1); + let end = Some(3); + + let data = DataSlice::new(bytes, start, end); + let expected = [2u8, 3]; + assert_eq!(&expected, data.get_bytes()); +} diff --git a/tests/unit/script/lib.rs b/tests/unit/script/lib.rs index 81c54e201f1..7996dae56cb 100644 --- a/tests/unit/script/lib.rs +++ b/tests/unit/script/lib.rs @@ -8,3 +8,6 @@ extern crate util; #[cfg(all(test, target_pointer_width = "64"))] mod size_of; #[cfg(test)] mod textinput; +#[cfg(test)] mod dom { + mod blob; +}