Don't allocate response objects to avoid triggering GC

[tobie]

On the list, @Maksims writes:

Garbage Collection - one of the major bottlenecks and high consideration for real-time applications. If application is targeting 60+ fps, it has to avoid any allocations and avoid calling any native JS methods that allocate response objects, for example getBoundingClientRect.
High GC loops do lead to periodical JS thread stalls, that can drop many frames, making 60fps unreachable.

[tobie]

The original API design as proposed by @rwaldron didn't have GC collection issues. The new design does, mainly because I wanted the same API to be available from the event object and the sensor instance itself.

Now that we've stopped exposing the sensor reading from the event object, maybe we should reconsider exposing it as a separate object on the sensor itself to avoid GC issues.

What if we tried something like that instead (here for gyroscope)?:

[SecureContext]
interface Sensor : EventTarget {
  readonly attribute SensorState state;
  readonly attribute DOMHighResTimeStamp timestamp;
  void start();
  void stop();
  attribute EventHandler onchange;
  attribute EventHandler onactivate;
  attribute EventHandler onerror;
};

dictionary SensorOptions {
  double? frequency;
};

[Constructor(SensorOptions options)]
interface Gyroscope : Sensor {
  serializer = { x, y, z, timestamp }; // so we can still get a reading object when needed
  readonly attribute unrestricted double? x;
  readonly attribute unrestricted double? y;
  readonly attribute unrestricted double? z;
};

That would allow uses like this:

let gyro = new Gyroscope({ frequency: 240 });
gyro.start();
    
gyro.onchange = _ => {
    console.log("Rotation rate around the X-axis " + gyro.x);
    console.log("Rotation rate around the Y-axis " + gyro.y);
    console.log("Rotation rate around the Z-axis " + gyro.z);
};

gyro.onerror = event => console.log(event.error.name, event.error.message)

Thoughts?

[Maksims]

@tobie, this design would allow getting state of data, without allocation on changes. And would allow to subscribe to data changes.
Which ticks both use cases: async and sync.
And prevents allocations.

This is actually is a good option here.

Although, this pattern will only work for singleton sensor types - there is only one gyro, and one accelerometer ever?
Does Sensors API assumes that there might be multiple sensors of the same type?
Lets say there are two controllers, like HTC Vive Controller, so you can have multiple attached. Does Sensors API covers this types of sensors too?

One slight concern, with sensors that have frequency that affects the "accumulated" data. For example accelerometer would not just get current state, but accumulated between pulls data? If that is the case, then with sync use cases, pulling could be sync too.

[tobie]

Although, this pattern will only work for singleton sensor types - there is only one gyro, and one accelerometer ever?
Does Sensors API assumes that there might be multiple sensors of the same type?
Lets say there are two controllers, like HTC Vive Controller, so you can have multiple attached. Does Sensors API covers this types of sensors too?

Yes, via identifying parameters which are sensor type specific, e.g. (completely made-up identifying parameters):

let gyro = new Gyroscope({
    frequency: 240,
    external: true, // identifying parameter
    hand: "right"   // identifying parameter
});

One slight concern, with sensors that have frequency that affects the "accumulated" data. For example accelerometer would not just get current state, but accumulated between pulls data? If that is the case, then with sync use cases, pulling could be sync too.

I've punted on dealing with accumulate date for now, tbh. I wan't to make sure we don't design something that makes this impossible in the future, however, so suggestions are welcome.

[Maksims]

Yes, via identifying parameters which are sensor type specific, e.g. (completely made-up identifying parameters):

That makes good sense. Providing default behaviour assuming it subscribes to "first", and allowing devs to enumerate available sensors and choose specific.

I've punted on dealing with accumulate date for now, tbh. I wan't to make sure we don't design something that makes this impossible in the future, however, so suggestions are welcome.

Indeed, it can be a tricky one. So for example with Accelerometer, current values of acceleration - is what will be available as state, but not the deltas. Developer has ability to calculate delta if he needs so in event handlers or in fixed update method (sync path).
Which makes sync polling not a need.

[tobie]

That makes good sense. Providing default behaviour assuming it subscribes to "first", and allowing devs to enumerate available sensors and choose specific.

Specific sensor specs get to define what default should be (or if a default makes sense at all).

[tobie]

Just to clarify, gyro.x is a getter for the last, asynchronously polled x value of the gyroscope, which is stored in a shared memory buffer. It is not a sync poller.

[Maksims]

Just to clarify, gyro.x is a getter for the last, asynchronously polled x value of the gyroscope, which is stored in a shared memory buffer. It is not a sync poller.

Indeed, this is just accessor to latest available data.
If it would pull actual data, it then shall be a method to clearly indicate that there is logic going on behind the scenes, as it would block JS thread probably.

[alexshalamov]

@tobie Initially we've implemented API in most efficient way. The Sensor.reading was a 'current' reading, thus, no need to create new reading on every update. After discussion under (w3c/ambient-light#15), you explained how you want API to behave and that SensorReading has to be recreated for every update.

So, I would propose to keep API as it is. If we want more FPS and less GC, we have to modify Sensor.reading description that will explain that Sensor.reading always points to current reading of the sensor.

'Sensor has reading' => interface Sensor { SensorReading reading; } (good design)
'Sensor is reading' => interface Sensor : SensorReading {} (bad design)

which is stored in a shared memory buffer. It is not a sync poller.

I know that Mikhail and I explained how we've implemented sensor reading update in Chromium, however, imo, we should avoid talking about UA specific implementation details.

[pozdnyakov]

Why not just keep the existing semantics (sensor.reading property) and say that it always returns the same object instead of re-creating it every round?
Mixing of sensor and it's data does not look beneficial.

[tobie]

Thanks for your input @alexshalamov. Getting back to our requirements, I think we want to try and fulfill the following, somewhat contradictory use-cases:

1. high-frequency low-latency use-cases (e.g. head-tracking for VR),
2. operating over multiple, sequential sensor readings in (near-) realtime (e.g. Kalman filters),
3. storing sensor reading for further processing at a later stage, either locally or remotely.

Requirement 1) is extremely perf sensitive, and if our ability to deliver on it is hindered because we're triggering too much GC, we should definitely fix that. Requirements 2) and 3) on the other hand require that we have distinct objects representing each readings (additionally, timestamps for 3) need to have an epoch-based component and not be DOMHighResolution only).

The design I'm suggesting here (which, btw, is just a suggestion), delivers on 1), but require an extra bit of work for 2) and 3); basically a way to snapshot the state of the sensor at t_n. I added a serializer in the above example as a placeholder. But that could also be a method, e.g.:

[SecureContext]
interface Sensor : EventTarget {
  readonly attribute SensorState state;
  void start();
  void stop();
  SensorReading snapshot();
  attribute EventHandler onchange;
  attribute EventHandler onactivate;
  attribute EventHandler onerror;
};

interface SensorReading {
  readonly attribute DOMHighResTimeStamp timeStamp;
  readonly attribute DOMHighResTimeStamp timeOrigin;
};

dictionary SensorOptions {
  double? frequency;
};

// E.g. for Gyroscope

interface GyroscopeValues {
  readonly attribute unrestricted double? x;
  readonly attribute unrestricted double? y;
  readonly attribute unrestricted double? z;
};

[Constructor(SensorOptions options)]
interface Gyroscope : Sensor {};
Gyroscope implements GyroscopeValues;

interface GyroscopeReading : SensorReading {};
GyroscopeReading implements GyroscopeValues;

We could even imagine building on top of this a buffer for all snapshots in between to event turns or some such.

[tobie]

Why not just keep the existing semantics (sensor.reading property) and say that it always returns the same object instead of re-creating it every round?

Because it doesn't help us with requirements 2) and 3) above, makes for more verbose code (sensor.reading.x instead of sensor.x), and makes mutable an object developers will/should think of as immutable.

[tobie]

@rwaldron would love your thoughts on the above. Thanks!

[pozdnyakov]

Because it doesn't help us with requirements 2) and 3) above, makes for more verbose code (sensor.reading.x instead of sensor.x)

As for 2) and 3) sensor.reading.x is not any worse than sensor.x (is it?), however sensor.x (y, z) would semantically mean position of the sensor itself, not the obtained data.

and makes mutable an object developers will/should think of as immutable.

cannot reading be readonly?

[tobie]

Because it doesn't help us with requirements 2) and 3) above, makes for more verbose code (sensor.reading.x instead of sensor.x)

As for 2) and 3) sensor.reading.x is not any worse than sensor.x (is it?)

Well for both cases 2) and 3) it seems developers would essentially play around with reading objects, so they woudl be doing: reading.x anyway.

however sensor.x (y, z) would semantically mean position of the sensor itself, not the obtained data.

Meh. I'm sure that won't be an issue for JS devs at all. There's a long tradition of favoring terseness over semantic purity in JS. They'll feel right at home with this.

and makes mutable an object developers will/should think of as immutable.

cannot reading be readonly?

Sure, but that still wouldn't resolve immutability issues. It's no longer a sensor reading if it changes over time. It's a proxy for the sensor values.

[Maksims]

If sensor has reading property, which is an object, with properties, then developer in event might assume it is immutable and/or instanced. This could lead to confusion like:

var lastReading = null;
accelerometer.onchange = function() {
    if (lastReading) {
        var deltaX = this.reading.x - lastReading.x;
    }
    lastReading = this.reading;
};

Someone would be confused why this wouldn't work on assumption that reading was immutable objects.

But if there is no object involved, such as sensor.x, then:

var lastX = null;
accelerometer.onchange = function() {
    if (lastX !== null) {
        var deltaX = this.x - lastX;
    }
    lastX = this.x;
};

There is much less chance to get confused in this case.
Bear in mind, examples above - are for async workflow. Sync workflow would access accelerometer data directly in fixed update function:

var lastX = null;
var update = function() {
    requestAnimationFrame(update);

    if (lastX !== null) {
        var deltaX = accelerometer.x - lastX;
    }
    lastX = accelerometer.x;
};
requestAnimationFrame(update);

[rwaldron]

But if there is no object involved, such as sensor.x, then:
...
There is much less chance to get confused in this case.

Yep, that's how J5 is designed as well.

---

I've never been a fan of the sensor.reading.foo design, but was always willing to accept that certain changes would need to be made to accomodate the platform. I'm happy to see that we're circling back before fully committing to the existing design.

The design that @tobie suggested above, which had this example:

let gyro = new Gyroscope({ frequency: 240 });
gyro.start();
    
gyro.onchange = _ => {
    console.log("Rotation rate around the X-axis " + gyro.x);
    console.log("Rotation rate around the Y-axis " + gyro.y);
    console.log("Rotation rate around the Z-axis " + gyro.z);
};

gyro.onerror = event => console.log(event.error.name, event.error.message);

Satisfies criteria 1, 2 and 3. Here's how:

1. high-frequency low-latency use-cases (e.g. head-tracking for VR),

We already know why, it's because there is no excessive allocation and therefore no excessive GC demands.

2. operating over multiple, sequential sensor readings in (near-) realtime (e.g. Kalman filters),
3. storing sensor reading for further processing at a later stage, either locally or remotely.

Application code can easily copy the values from the properties and store them for later, if they want to do so:

let cache = [];
gyro.onchange = _ => {
    cache.push({ x, y, z } = gyro);
};

... Any amount of historic data can be kept or discarded as needed by the application (re: criteria 2)

[tobie]

Application code can easily copy the values from the properties and store them for later, if they want to do so

@rwaldron, seems you're suggesting ES6/7 provides enough syntactic sugar to make a gyro.snapshot() method (placeholder name) redundant. We can always add it at a later stage if lots of people ask for it.

[rwaldron]

seems you're suggesting ES6/7 provides enough syntactic sugar to make a gyro.snapshot()

Definitely

[alexshalamov]

@Maksims do you have any test set (data) which shows that current approach makes 60fps target unreachable? I quckly measured codepath for sensor reading update using current API and with modified codepath that matches proposal in this issue. The difference between two is neglectable: fluctuations on a level of 0.1-1 microseconds per reading update. Tested on Mi Pad2 device.

@tobie

Requirement 1) is extremely perf sensitive, and if our ability to deliver on it is hindered because we're triggering too much GC, we should definitely fix that.

Before changing the API, we need some data which proves that performance issues do exist, otherwise, it looks like a premature optimisation.

[tobie]

Garbage collection pauses causing missed frames are well known and well documented issues. Note the issue isn't the extra allocation time, it's the GC pauses.

Among the TAG requests (coming from @slightlyoff) was a requirement to make this available off of the main thread for fear of jank.

Designing CPU-hungry APIs designed for perf-sensitive applications with performance in mind isn't premature optimization, it's just Doing the Right Thing[TM]. ;)

That said, I'd love to see test data if there's some easily available, but I don't think the onus is on the OP to prove his point here, rather it's on us to explain how the current design is going to avoid causing GC pauses when GC kicks in.

[Maksims]

@alexshalamov @tobie - have answered pretty well.
GC stalls is very known problem working with real-time apps.
Current design on it's own wont lead to long GC, but it makes GC longer, which means there is less GC "buffer" for application logic. If application will go over that "buffer" of what GC can keep up with, it will get into Major GC long stalls and freeze every second or so. With only one solution - refresh the tab.
Remember that GC is shared between tabs of same JS thread (when opening pages in new tabs within same domain). This means multiple apps will contribute even more to GC.

Every little thing matters. And if API does not provides a way to avoid allocations then developers hitting the GC issues will struggle to deal with a situation.

As @tobie said:

Designing CPU-hungry APIs designed for perf-sensitive applications with performance in mind isn't premature optimization, it's just Doing the Right Thing[TM]. ;)

[alexshalamov]

@tobie There are few concerns with proposed modifications.

• When control and data interfaces are merged, extensibility is lost. For example, for sensors that can provide additional data in 'uncalibrated / raw mode' either new interface should be created, or all possible data fields have to be present on main interface.

• snapshot() makes new object, I don’t see any difference to what we have at the moment.

• timeStamp is only accessible through snapshot() => for algorithms that use timestamp, performance would be the same as with the current API.

Chrome canary has implementation of ambient light, accelerometer, gyroscope and magnetometer. It would be good to have measurements that prove that the current approach would not allow us to reach continuous 60fps target. If required, I can quickly prototype infinite scroller with recyclable elements, so that we will see whether current approach affecting scrolling performance / fps or not.

If you still think that GC would be a problem, we could consider an alternative solution. Could we instead provide a method that will fill sensor reading data, e.g. Sensor.getReading(Dictionary out); or Sensor.read(Dictionary out);? This will allow to reuse object, yet, main interface is not polluted with unwanted fields and extensibility is not lost.

[pozdnyakov]

Looks like constructing of small objects (like sensor readings) once per animation frame does not actually cause GC issues: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/NJFiEsqW-RM

[tobie]

Thanks for looking into this. After a cursory glance at the comments on the Chromium thread, it doesn't seem to address the case where you have more than one reading per frame, what happens when these objects are kept around longer than a frame (Hannes' comment about GC easily taking "several milliseconds" in certain cases is particularly worrying), or whether that has an impact in a larger application settings (which is what the OP was concerned about).

It would also be important to get feedback from other implementors on this topic (which might have different GC strategies).

[alexshalamov]

Thanks for looking into this. After a cursory glance at the comments on the Chromium thread, it doesn't seem to address the case where you have more than one reading per frame

This would not trigger GC stalls, since sensors of the same type will share reading, thus max number of updated readings per frame would be rather small, and each allocation is also insignificant as I mentioned earlier (less than microsecond)

when these objects are kept around longer than a frame (Hannes' comment about GC easily taking "several milliseconds" in certain cases is particularly worrying)

These objects cannot be kept for a long time, either they are GC'd after each frame, or when sensor is stopped, then internal reading is released and the one that is reffed by the user, will be GC'd when reference is lost. It would be worrying if we would keep huge internal cache of readings for a long time and then invalidate them all at once, which is not the case for our implementation.

It would also be important to get feedback from other implementors on this topic (which might have different GC strategies).

Very good point, but taking in to account performance of JSCore or SpiderMonkey, I doubt this will be an issue.

[tobie]

Thanks for looking into this. After a cursory glance at the comments on the Chromium thread, it doesn't seem to address the case where you have more than one reading per frame

This would not trigger GC stalls, since sensors of the same type will share reading, thus max number of updated readings per frame would be rather small, and each allocation is also insignificant as I mentioned earlier (less than microsecond)

A microsecond millisecond is 6% of a frame's total budget. That clearly doesn't fall in the insignificant category, especially if there are multiple of them, and particularly on cheaper mobile devices. (Edit: unclear if GC is < 1ms as mentioned on the chromium thread or < 1μs as @alexshalamov claims here. I assumed the former even though I incorrectly wrote the latter.)

when these objects are kept around longer than a frame (Hannes' comment about GC easily taking "several milliseconds" in certain cases is particularly worrying)

These objects cannot be kept for a long time, either they are GC'd after each frame, or when sensor is stopped, then internal reading is released and the one that is reffed by the user, will be GC'd when reference is lost. It would be worrying if we would keep huge internal cache of readings for a long time and then invalidate them all at once, which is not the case for our implementation.

This assumes that no application-level code interferes with this. Consider the following (inspired by a use case found in a related issue):

var buffer =[];
var sensor = new Accelerometer({ frequency: 120 });

sensor.onchange = _ => { 
    buffer.push(sensor.reading);
    if (buffer.length >= 500) {
        fetch("/log-data", {
            method: "POST",
            body: JSON.stringify(buffer)
        });
        buffer.length = 0;
    }
};

This also doesn't address the concern of GC issues in real life scenarios, which is what the OP was worried about.

It would also be important to get feedback from other implementors on this topic (which might have different GC strategies).
Very good point, but taking in to account performance of JSCore or SpiderMonkey, I doubt this will be an issue.

I'd like to hear those implementors speak for themselves.

[rwaldron]

A microsecond is 6% of a frame's total budget.

Serious question: microsecond or millisecond?

[tobie]

A microsecond is 6% of a frame's total budget.
Serious question: microsecond or millisecond?

The thread on the chromium list talks about milliseconds. It's milliseconds I intended to write here. But maybe that thread is incorrect? Or @alexshalamov above is?

[alexshalamov]

The thread on the chromium list talks about milliseconds.

I never said milliseconds in blink-dev, neither in this thread.

This assumes that no application-level code interferes with this. Consider the following (inspired by a use case found in a related issue):

This is not a valid example, in the same way developers can populate buffer using buffer.push({sensor.x; sensor.y; sensor.z}); and then try invalidating it during critical rendering path, e.g., re-layouting.

Based on profiling data for different code paths and initial feedback from developers who are implementing GC in V8 / blink, there is no technical proof that exposing objects instead of primitives would cause "stop-the-world" JS thread stalls because of GC.

Anyways, the change for the interfaces was already done, unfortunately, without any data or even JS example that represents the problem. I think we should avoid that in the future and do proper investigation before rushing to change everything.

[alexshalamov]

(Edit: unclear if GC is < 1ms as mentioned on the chromium thread or < 1μs as @alexshalamov claims here. I assumed the former even though I incorrectly wrote the latter.)

I never said that GC is less than microsecond either, I said that reading update, e.g., re-creating reading object (3 doubles, 12 bytes) will add extra 0.1-1 microsecond overhead.

(Edit) - actually, it can be even faster if for the test, we would have used pre-allocated buffer and then placement new.

[tobie]

I never said milliseconds in blink-dev, neither in this thread.

I was quoting Hannes Payer who said:

An allocation in a time critical time frame may cause garbage collection but that does not mean that this is an issue. Garbage collection is really fast (<1ms) if most recently allocated objects die soon after their allocation. Otherwise it can easily take several milliseconds but we are actively working on reducing this cost even further.

So it would still be useful to get some clarity here. Which is it?

[tobie]

Anyways, the change for the interfaces was already done, unfortunately, without any data or even JS example that represents the problem. I think we should avoid that in the future and do proper investigation before rushing to change everything.

As explained when that changed was done, the sensor.reading was a departure from the original design to accommodate referencing the same object from both the sensor and the event. Once the WG decided referencing readings from the event was a bad idea, there was no longer any technical reason to this design that I was aware of, plus a high risk of GC issues given prior experience working with JS-heavy apps on mobile devices.

That said, if we have a new need for this, it makes complete sense re-exploring, but we'll need serious testing to be absolutely sure this doesn't create GC issues on any platform. Fwiw, I'm absolutely not convinced by your test case given the kind of cases where I've seen such issues crop-up in the past.

This assumes that no application-level code interferes with this. Consider the following (inspired by a use case found in a related issue):

This is not a valid example, in the same way developers can populate buffer using buffer.push({sensor.x; sensor.y; sensor.z}); and then try invalidating it during critical rendering path, e.g., re-layouting.

You're right from a theoretical perspective. But in practice, a JS dev not attuned to the inner workings of Chrome GC, will probably assume buffer.push({sensor.x; sensor.y; sensor.z}); has a GC cost while buffer.push(sensor.reading); doesn't "as the object was already created." (That would have been my assumption.)

That said, that begs the question as to whether we really want to expose sensor.x, sensor.y, and sensor.z on high-frequency sensors (I have no idea—I'm just thinking out loud).

[alexshalamov]

I was quoting Hannes Payer who said:

Ah, ok. I was thinking I made typo somewhere. I completely agree that if reading update would cause 1ms GC sweeps, it would be unacceptable. It would be visible in inspector js profiler.

[tobie]

I was thinking I made typo somewhere. I completely agree that if reading update would cause 1ms GC sweeps, it would be unacceptable. It would be visible in inspector js profiler.

Cool. Glad we're on the same page.

[Maksims]

The "readings object" pattern across different Sensors API, will lead to different sensors of allocating various objects. Those might be a from single double (couple of bytes) to large objects, similar to touch events or mouse events. Naive way to store short-term or even long term history, would be by storing reading objects. Following such pattern this would lead to significant GC for sensors that have more than just few doubles, especially with referenced targets or more complex objects. This would lead to much bigger Retained Sizes associated with readings objects that keep references such as for DOM targets and other things.

By having no readings object, there is no association cost and no way to store those readings objects. So if developer need to store history - then he stores what he needs. There is no multiple ways to go about it, it is straight forward with no misinterpretations.

This practice avoids that retained memory size paths, and defines good basis for API design which has no different use paths and does not require to learn certain way to avoid issue in case if they occur, which in big applications is hard to track because there are consequence of many many tangled allocations.

@alexshalamov, I see you mostly "resistive" against those changes. But I couldn't find the actual motivation behind such resistance. Does no "readings object" path makes things much worse in your opinion, and what are those "worse" things? Is it technical reasoning or more of a conceptual/design. I can't see how "readings object" API design is better to be honest, listing reasoning above - it leads to multiple ways of doing things (referencing readings object or taking data from reading objects), and leads to Retained Size issues, especially with other Sensor API's that might have much more data provided. And especially taking in account potentially not all readings data needs to be changed on each reading.

Could you please reason the "resistance" you show?

[alexshalamov]

@Maksims I'm not resisting anything, I'm for simpler and faster API and its implementation. If you would check the implementation in chromium you will understand that we have the same goal.

I'm +1 for the removal of the intermediate reading object, it reduced implementation complexity in chromium as well as API surface area.

[alexshalamov]

@pozdnyakov We would need to investigate and re-measure performance impact when we have batching mode. Moving to Level 2.