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.

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

Find a way to do less copying when sending large typed data #34796

Closed
alexmarkov opened this issue Oct 15, 2018 · 15 comments
Closed

Find a way to do less copying when sending large typed data #34796

alexmarkov opened this issue Oct 15, 2018 · 15 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@alexmarkov
Copy link
Contributor

When sending a large typed data through a message port, there is an optimization to convert it to an external typed data (

if ((kind == Snapshot::kMessage) &&
(bytes >= kExternalizeTypedDataThreshold)) {
// Write as external.
writer->WriteIndexedObject(external_cid);
writer->WriteTags(writer->GetObjectTags(this));
writer->Write<RawObject*>(ptr()->length_);
uint8_t* data = reinterpret_cast<uint8_t*>(ptr()->data());
void* passed_data = malloc(bytes);
if (passed_data == NULL) {
OUT_OF_MEMORY();
}
memmove(passed_data, data, bytes);
static_cast<MessageWriter*>(writer)->finalizable_data()->Put(
bytes,
passed_data, // data
passed_data, // peer,
IsolateMessageTypedDataFinalizer);
) to avoid copying on the receiver side. This morphs object from one type to another, which causes problems flutter/flutter#22796, #34752.

As a workaround to these problems, the optimization of converting typed data to external will be put under a flag and disabled by default. However, we still need to find a way to do less copying when sending large amount of data through message ports, preferably without morphing object types.

@alexmarkov alexmarkov added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Oct 15, 2018
dart-bot pushed a commit that referenced this issue Oct 15, 2018
…through a message port

Closes #34752
Issue flutter/flutter#22796
Issue #34796

Change-Id: Ia769ee7d828f37a5ca36fedebcf609c3bab8c38c
Reviewed-on: https://dart-review.googlesource.com/c/79825
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Auto-Submit: Alexander Markov <alexmarkov@google.com>
@a-siva
Copy link
Contributor

a-siva commented Oct 15, 2018

The original issue that triggered this optimization is here #31959

@hooluupog
Copy link

If Dart isolate support transferable Data. #4149

@mraleph
Copy link
Member

mraleph commented Oct 21, 2018

Note that transferrables on the Web are implemented by externalizing typed arrays behind the scenes. This is the kind of implementation we want to avoid, because it introduces the problem that type of the object changes dynamically: you take a normal typed array and send it to another worker - it suddenly becomes an empty typed array on the sender side.

@alexmarkov
Copy link
Contributor Author

alexmarkov commented Oct 22, 2018

Maybe we can introduce transferable typed data as separate classes (they could be private in dart:typed_data), created with separate public factory constructors (e.g. Int8List.transferable(int length)). Unlike other typed data classes, transferable could have a mutable length (if needed), or an extra flag indicating that contents was moved to another isolate and should not be accessed anymore. This way it will be possible to avoid both extra copying and dynamic changes of object types.

The disadvantage of this approach is that users will need to change their code and use the new API to create transferable typed data.

/cc @aam

@yjbanov
Copy link

yjbanov commented Oct 24, 2018

you take a normal typed array and send it to another worker - it suddenly becomes an empty typed array on the sender side

Why is this a problem?

@mraleph
Copy link
Member

mraleph commented Oct 24, 2018

Because right now we can treat type array length as a constant value - and optimize based on that, independent of whether it escape anywhere or not. You are loosing this property if you start changing array length when you send the data over. e.g. right now

class Matrix3D {
  final storage = new Float32Array(9)
}

Matrix3D smth;

smth.storage[1];  // No bounds check needed. Static guarantees of length

@matanlurey
Copy link
Contributor

You could just "0-out" the array, instead of clearing all elements, right?

@yjbanov
Copy link

yjbanov commented Oct 24, 2018

we can treat type array length as a constant value

Ah, nice! Didn't know you did that.

You could just "0-out" the array, instead of clearing all elements, right?

That's is as bad as copying :)

@matanlurey
Copy link
Contributor

FWIW, in AOT mode, it might be reasonable to segfault if you attempt to access a typed array that has been transferred. In JIT mode you could throw an exception? I don't know if that makes sense, but I imagine the number of folks that will be using transferable arrays is rather small, and it is probably OK to give them undefined behavior if you transfer the array and then try to use it.

@yjbanov
Copy link

yjbanov commented Oct 24, 2018

it might be reasonable to segfault if you attempt to access a typed array that has been transferred

That means inserting a check before every access to a typed array. Since typed arrays tend to be used in performance-critical parts of the code, I'd try to avoid adding checks.

@matanlurey
Copy link
Contributor

Oh, I mean don't insert a check, and let the program just crash :)

@yjbanov
Copy link

yjbanov commented Oct 24, 2018

Oh, I mean don't insert a check, and let the program just crash :)

The thing is, it won't crash. Because the memory is not copied/moved anywhere, it's still there. So the crash is not guaranteed to happen predictably.

@matanlurey
Copy link
Contributor

Hmm. Could the pointer to the array be pointed to a null location, or something? 💥

@yjbanov
Copy link

yjbanov commented Oct 24, 2018

Could the pointer to the array be pointed to a null location, or something?

That's a @mraleph question. Your isolate could have multiple pointers to the array. You'd need some indirection for that to work, which, again, adds overhead.

dart-bot pushed a commit that referenced this issue Mar 15, 2019
This is the first step towards unification of TypedData and
ExternalTypedData.

Issue: #34796
Change-Id: I8aec72030e251f9cd9f00fc5073f8e42b83a8d17
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/79430
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
dart-bot pushed a commit that referenced this issue Mar 18, 2019
This reverts commit d1085e8.

Reason for revert: performance regressions.

Original change's description:
> [vm] Add data field to TypedData
> 
> This is the first step towards unification of TypedData and
> ExternalTypedData.
> 
> Issue: #34796
> Change-Id: I8aec72030e251f9cd9f00fc5073f8e42b83a8d17
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/79430
> Commit-Queue: Alexander Markov <alexmarkov@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
> Reviewed-by: Ryan Macnak <rmacnak@google.com>

TBR=vegorov@google.com,kustermann@google.com,rmacnak@google.com,alexmarkov@google.com,regis@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Issue: #34796
Change-Id: Ia11d8c3af0af29d49c3dbf36a3fe7041fbf9aae3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/97166
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
tekknolagi pushed a commit to tekknolagi/dart-assembler that referenced this issue Nov 3, 2020
This is the first step towards unification of TypedData and
ExternalTypedData.

Issue: dart-lang#34796
Change-Id: I8aec72030e251f9cd9f00fc5073f8e42b83a8d17
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/79430
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
tekknolagi pushed a commit to tekknolagi/dart-assembler that referenced this issue Nov 3, 2020
This reverts commit d1085e8.

Reason for revert: performance regressions.

Original change's description:
> [vm] Add data field to TypedData
> 
> This is the first step towards unification of TypedData and
> ExternalTypedData.
> 
> Issue: dart-lang#34796
> Change-Id: I8aec72030e251f9cd9f00fc5073f8e42b83a8d17
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/79430
> Commit-Queue: Alexander Markov <alexmarkov@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
> Reviewed-by: Ryan Macnak <rmacnak@google.com>

TBR=vegorov@google.com,kustermann@google.com,rmacnak@google.com,alexmarkov@google.com,regis@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Issue: dart-lang#34796
Change-Id: Ia11d8c3af0af29d49c3dbf36a3fe7041fbf9aae3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/97166
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
@rmacnak-google
Copy link
Contributor

TransferableTypedData was introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

8 participants