Store event handler IDs as longs. Fixes #12058 #12305

SteveSandersonMS · 2019-07-18T12:30:34Z

We had triaged #12058 for 3.1, but I realised that it would actually impact the public API surface in a bunch of places, not only RenderTreeFrame in the pubternal zone. To avoid us getting stuck with this later, I thought it safer to do right away - it's not taken much time.

Also, it's a change that includes some risk because of how many different things have to be updated to line up with this, so if we are going to do it, it really should be in preview 8 to give chance to react to any bugs we discover.

@javiercn We talked about us looking at some parts of this together, so if you're interested please ping me and I'll explain the details of how the memory layout matters and what constraints there are on it.

SteveSandersonMS · 2019-07-18T12:32:16Z

src/Components/Components/src/RenderTree/RenderTreeFrame.cs

@@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.Components.RenderTree
    /// <summary>
    /// Represents an entry in a tree of user interface (UI) items.
    /// </summary>
-    [StructLayout(LayoutKind.Explicit)]
+    [StructLayout(LayoutKind.Explicit, Pack = 4)]


Pack = 4 is now needed because by default, Mono picks a struct length that's the smallest multiple of the largest field size. The largest field is now 8 bytes, so it would pick struct length 40. Explicitly specifying Pack=4 lets it pick a multiple of 4, so with this it picks struct length 36.

The existing TS-side code already expects 36. We could change it to 40 but with this line, there's no need.

SteveSandersonMS · 2019-07-18T12:36:45Z

src/Components/Server/src/Circuits/RenderBatchWriter.cs

@@ -130,16 +130,15 @@ int Write(in ArrayRange<RenderTreeFrame> frames)

        void Write(in RenderTreeFrame frame)


Unfortunately, for server-side Blazor, this means reference frames now take 20 bytes each instead of 16 before, and in most of the frame types, a large part of those 20 bytes are just padding.

We could reduce it to 14 bytes per frame by:

Writing the FrameType as a 2-byte value instead of 4 bytes. It's a short anyway. This would save 2 bytes.

Having a separate FrameType value for attribute frames that have a nonempty event handler ID. Then for that new frame type, we wouldn't need to write out the AttributeValue (which is always left blank in this case). This would let all the possible frame type bodies fit into 12 bytes, and hence saves 4 bytes.

However I'm not doing this now because it can be done non-breakingly in the future if we want. Also, making this data as hyper-packed as possible may turn out to be counterproductive because if we ever need to add any new data, that would trip the field size and force us to re-expand again, which would be disruptive.

Filed #12306 and put it right on the backlog

Sounds good! I was going to suggest to make the frame type a byte directly as I wouldn't expect us to have more than 256 frame types, but I realize that would disrupt the alignment and possibly slow things down.

javiercn · 2019-07-18T13:24:54Z

src/Components/Blazor/Blazor/src/Rendering/WebAssemblyRenderer.cs

@@ -120,7 +120,7 @@ protected override void HandleException(Exception exception)
        }

        /// <inheritdoc />
-        public override Task DispatchEventAsync(int eventHandlerId, EventFieldInfo eventFieldInfo, UIEventArgs eventArgs)
+        public override Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo eventFieldInfo, UIEventArgs eventArgs)


I know our events are always positive and that we are never going to wrap around an ulong, but I'm worried about using unsigned types in here just because we pretty much don't ever use them and because I'm not confident on all the integration issues this might bring, from serialization, to method invocation, etc. Id rather if possible we simply use a long value as it achieves our goal and is less likely to cause issues in the future.

That said, It might be a case of me being paranoid about this for no reason. I'm just scared of the unknowns using this brings, as I don't think there's an API (or very few APIs) in ASP.NET or .NET that specifically use them.

I picked ulong because it was easier to make robust:

Ints don’t implicitly convert to or from it (unlike long), so you get compile time checking that we’re consistent everywhere in .NET

The JS-side code for reading uint64 is simpler and harder to get wrong than if it was reading int64, as there’s no sign bit to account for.

That said, It might be a case of me being paranoid about this for no reason. I'm just scared of the unknowns using this brings, as I don't think there's an API (or very few APIs) in ASP.NET or .NET that specifically use them.

I don't share your concerns 😆 I've used ulong before.

javiercn · 2019-07-18T13:28:04Z

src/Components/Components/src/Rendering/RenderBatchBuilder.cs

@@ -18,7 +18,7 @@ internal class RenderBatchBuilder : IDisposable
        // Primary result data
        public ArrayBuilder<RenderTreeDiff> UpdatedComponentDiffs { get; } = new ArrayBuilder<RenderTreeDiff>();
        public ArrayBuilder<int> DisposedComponentIds { get; } = new ArrayBuilder<int>();


Should component ids be on the same boat? I can imagine that if we don't want to ever wrap around an event, it might be even more important for components, as it might be the case that 1 event triggers multiple component instantiations.

Thoughts?

I know the struct layout might get more complicated if we add 4 more bytes. But I think its a matter of being better safe than sorry.

I’m open to it, but it will be quite a lot more disruptive than the event handler ID change, as component IDs are used in so many more places.

To run out of IDs, you’d need to do something like instantiate 1000 components per second continuously for 25 days (in one circuit). I think we could say that is impractical for lots of reasons.

One cheaper option would just be to change any public API for component IDs to be ulong, which leaves us the option of changing the underlying storage in the future if we want.

javiercn · 2019-07-18T13:34:49Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

+    const address = (baseAddress as any as number) + (fieldOffset || 0);
+    const heapU32Index = address >> 2;
+    return Module.HEAPU32[heapU32Index] + Module.HEAPU32[heapU32Index + 1] * uint64HighOrderShift;
+  },


Do we need to check for overflow here?
Number.MAX_SAFE_NUMBER is (2**53 - 1) which we are never going to hit, but it might be good to have a check to detect errors?

We could also file a bug to switch to BigInt when it becomes official. Chrome, (new Edge), and Firefox support it, and i'm sure there must be polyfills transforms for it if we want to be backwards compat.

I’d rather not slow down every uint64 read for this. We know we’ll never reach 2^53 in normal use so it would only be useful for detecting some kind of memory corruption bug. If the memory is corrupted in even the slightest way, dozens of other things blow up.

Actually I changed my mind. This particular function isn't specific to event handler IDs, but rather is just a general way of reading a uint64 value. So I agree it is valuable to have a way of noticing if we ever start trying to use it to read something it can't read (e.g., .NET's uint64.MaxValue).

Will add a check.

javiercn · 2019-07-18T13:40:45Z

src/Components/test/testassets/ComponentsApp.Server/Startup.cs

-            services.AddServerSideBlazor();
+            services.AddServerSideBlazor(options =>
+            {
+                options.JSInteropDetailedErrors = true;


Left over? I don't care if you leave it here tbh as we don't do any exception testing here.

It was deliberate - I think this benefits us all.

javiercn

Looks great!

SteveSandersonMS added 3 commits July 18, 2019 12:54

Change event handler IDs to be longs

86b5aeb

Update unit tests

d5ecb78

Enable detailed errors for test app

c901793

SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Jul 18, 2019

SteveSandersonMS added this to the 3.0.0-preview8 milestone Jul 18, 2019

SteveSandersonMS requested review from rynowak and javiercn July 18, 2019 12:30

SteveSandersonMS commented Jul 18, 2019

View reviewed changes

javiercn reviewed Jul 18, 2019

View reviewed changes

SteveSandersonMS added 2 commits July 18, 2019 15:22

CR: Explicitly check if we exceed Number.MAX_SAFE_INTEGER

2ccf583

Update ref assemblies

709761d

javiercn approved these changes Jul 18, 2019

View reviewed changes

SteveSandersonMS merged commit 4e04b81 into master Jul 18, 2019

ghost deleted the stevesa/long-event-handler-ids branch July 18, 2019 16:53

mkArtakMSFT added the tell-mode Indicates a PR which is being merged during tell-mode label Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store event handler IDs as longs. Fixes #12058 #12305

Store event handler IDs as longs. Fixes #12058 #12305

SteveSandersonMS commented Jul 18, 2019

SteveSandersonMS Jul 18, 2019

SteveSandersonMS Jul 18, 2019 •

edited

Loading

SteveSandersonMS Jul 18, 2019

javiercn Jul 18, 2019

javiercn Jul 18, 2019

SteveSandersonMS Jul 18, 2019

rynowak Jul 18, 2019

javiercn Jul 18, 2019

SteveSandersonMS Jul 18, 2019

javiercn Jul 18, 2019

javiercn Jul 18, 2019

SteveSandersonMS Jul 18, 2019

SteveSandersonMS Jul 18, 2019

javiercn Jul 18, 2019

SteveSandersonMS Jul 18, 2019

javiercn left a comment

		@@ -130,16 +130,15 @@ int Write(in ArrayRange<RenderTreeFrame> frames)

		void Write(in RenderTreeFrame frame)

Store event handler IDs as longs. Fixes #12058 #12305

Store event handler IDs as longs. Fixes #12058 #12305

Conversation

SteveSandersonMS commented Jul 18, 2019

Choose a reason for hiding this comment

SteveSandersonMS Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

SteveSandersonMS Jul 18, 2019 •

edited

Loading