-
Notifications
You must be signed in to change notification settings - Fork 80
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
BarrageMessageProducer: Do Not Always Record Stream Table Rows #3857
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that the UI is still slow; aren't we sending much smaller updates to the web UI?
Potentially not related, but I'm also seeing some failures when this table is reversed (I'm assuming the web UI applies a table.reverse(); I'm assuming "reverse viewport" is something else?)
2023-05-22T18:23:30.895Z | EFAULT.refreshThread | ERROR | rumentedTableListenerBase | Uncaught exception for entry= reverse(), added.size()=50025900, modified.size()=0, removed.size()=50009200, shifted.size()=0, modifiedColumnSe
t={EMPTY}:
io.deephaven.base.verify.AssertionFailure: Assertion failed: asserted update.shifted.empty().
at io.deephaven.base.verify.Assert.fail(Assert.java:86)
at io.deephaven.base.verify.Assert.assertion(Assert.java:116)
at io.deephaven.engine.table.impl.BaseTable.notifyListeners(BaseTable.java:637)
at io.deephaven.engine.table.impl.ReverseOperation.onUpdate(ReverseOperation.java:225)
at io.deephaven.engine.table.impl.ReverseOperation$2.onUpdate(ReverseOperation.java:104)
at io.deephaven.engine.table.impl.InstrumentedTableUpdateListener$Notification.lambda$run$0(InstrumentedTableUpdateListener.java:37)
at io.deephaven.engine.table.impl.InstrumentedTableListenerBase$NotificationBase.doRunInternal(InstrumentedTableListenerBase.java:283)
at io.deephaven.engine.table.impl.InstrumentedTableListenerBase$NotificationBase.doRun(InstrumentedTableListenerBase.java:261)
at io.deephaven.engine.table.impl.InstrumentedTableUpdateListener$Notification.run(InstrumentedTableUpdateListener.java:37)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.runNotification(UpdateGraphProcessor.java:1285)
at io.deephaven.engine.updategraph.UpdateGraphProcessor$QueueNotificationProcessor.doWork(UpdateGraphProcessor.java:1461)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.flushNormalNotificationsAndCompleteCycle(UpdateGraphProcessor.java:1164)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.flushNotificationsAndCompleteCycle(UpdateGraphProcessor.java:1109)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.lambda$doRefresh$11(UpdateGraphProcessor.java:1744)
at io.deephaven.util.locks.FunctionalLock.doLocked(FunctionalLock.java:33)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.doRefresh(UpdateGraphProcessor.java:1732)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.refreshAllTables(UpdateGraphProcessor.java:1719)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.refreshTablesAndFlushNotifications(UpdateGraphProcessor.java:1573)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.lambda$new$3(UpdateGraphProcessor.java:270)
at java.lang.Thread.run(Thread.java:829)
2023-05-22T18:23:30.897Z | EFAULT.refreshThread | ERROR | .AsyncClientErrorNotifier | Error in table update: io.deephaven.base.verify.AssertionFailure: Assertion failed: asserted update.shifted.empty().
at io.deephaven.base.verify.Assert.fail(Assert.java:86)
at io.deephaven.base.verify.Assert.assertion(Assert.java:116)
at io.deephaven.engine.table.impl.BaseTable.notifyListeners(BaseTable.java:637)
at io.deephaven.engine.table.impl.ReverseOperation.onUpdate(ReverseOperation.java:225)
at io.deephaven.engine.table.impl.ReverseOperation$2.onUpdate(ReverseOperation.java:104)
at io.deephaven.engine.table.impl.InstrumentedTableUpdateListener$Notification.lambda$run$0(InstrumentedTableUpdateListener.java:37)
at io.deephaven.engine.table.impl.InstrumentedTableListenerBase$NotificationBase.doRunInternal(InstrumentedTableListenerBase.java:283)
at io.deephaven.engine.table.impl.InstrumentedTableListenerBase$NotificationBase.doRun(InstrumentedTableListenerBase.java:261)
at io.deephaven.engine.table.impl.InstrumentedTableUpdateListener$Notification.run(InstrumentedTableUpdateListener.java:37)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.runNotification(UpdateGraphProcessor.java:1285)
at io.deephaven.engine.updategraph.UpdateGraphProcessor$QueueNotificationProcessor.doWork(UpdateGraphProcessor.java:1461)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.flushNormalNotificationsAndCompleteCycle(UpdateGraphProcessor.java:1164)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.flushNotificationsAndCompleteCycle(UpdateGraphProcessor.java:1109)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.lambda$doRefresh$11(UpdateGraphProcessor.java:1744)
at io.deephaven.util.locks.FunctionalLock.doLocked(FunctionalLock.java:33)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.doRefresh(UpdateGraphProcessor.java:1732)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.refreshAllTables(UpdateGraphProcessor.java:1719)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.refreshTablesAndFlushNotifications(UpdateGraphProcessor.java:1573)
at io.deephaven.engine.updategraph.UpdateGraphProcessor.lambda$new$3(UpdateGraphProcessor.java:270)
at java.lang.Thread.run(Thread.java:829)
e1b6bbc
to
07a9d6a
Compare
I've filed the previous stack trace as #3858 (which is reproducible on 0.24.1 release). Here's another stack trace specific to the BarrageMessageProducer I've created with your latest commit:
I'm not sure if this is the same bug as reverse, or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this as-is, once you fix the invert bug you identified. Maybe consider assertions for !reverse when accumulating deltes, and for no-mods.
server/src/main/java/io/deephaven/server/barrage/BarrageMessageProducer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caught up
This one I was able to reproduce and should be all fixed now. Things appear to work AFAICT. I'm rebasing and fixing up for the |
Tested manually using reproducer provided by Devin:
I also ran Barrage Message Round Trip Tests.
The UI performance gets hit pretty hard by the 20ns period. I'm a little surprised by this, but decreasing to 1us is sufficient to eliminate the performance issue.