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

perf: de-prioritize rendering non-visible tiles #10183

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sgothel
Copy link

@sgothel sgothel commented Oct 7, 2024

Summary

Only render invisible combined-tiles if IDLE

As an attempt to increase responsiveness for client,
we only send invisible requested combined-tiles if IDLE.
IDLE is determined if socket poll timed out, i.e. nothing else to do for client.

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

@sgothel sgothel requested review from mmeeks and caolanm October 7, 2024 04:03
@sgothel sgothel self-assigned this Oct 7, 2024
@sgothel
Copy link
Author

sgothel commented Oct 7, 2024

The TileCombined's dimension, i.e. its _width and _height, is not fixed in the streaming TileCombined ctor (line ~404).
Only the 1st TileDesc dimenstion is being stored.
The combined dimension seems to be used at doRender only?
If fixed, i.e. AABBox style grown (-> Rectangle) we simply could use it to determine visibility .. instead of looping through all recreated TileDesc instances.

@sgothel
Copy link
Author

sgothel commented Oct 7, 2024

I need clarification regarding the IDLE criteria, which shall allow sending the invisible tiles.
(timeout = to)
This patch uses the to_left < time_consumed in drainedQueue, which works if initial to is reasonable reflecting time left.
Testing showed that a to > 0 is only given for IDLE periods?

@sgothel sgothel force-pushed the private/sgothel/cool_9989_deprio_rendering_invisible_tiles branch from 50a1781 to 2841348 Compare October 7, 2024 14:11
@sgothel
Copy link
Author

sgothel commented Oct 7, 2024

Force push resolved merge conflict with latest master tip as detected with the bots. I must have pushed it this morning in-between.

Copy link
Contributor

@mmeeks mmeeks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, some useful cleanups here perhaps - though the algorithm used doesn't seem to match what was asked for. Let me paste that again:

a) only render & send those that are inside a visible area and
b) re-spin the main-loop and
c) only render non-visible tiles when there is nothing else to do =)

Was there a problem with detecting nothing else to do in c) - I would expect that is reasonably easy to detect - when we found nothing to read from our remote socket, and had a non-zero timeout to poll for.

"an attempt to reduce bandwidth to client" - this is primarily a way of reducing latency, by not spending time rendering not-yet-seen tiles - and prioritizing visible tiles over non-visible ones. Nothing to do with bandwidth in particular.

common/Rectangle.hpp Outdated Show resolved Hide resolved
common/Rectangle.hpp Outdated Show resolved Hide resolved
kit/ChildSession.cpp Outdated Show resolved Hide resolved
kit/Kit.cpp Outdated Show resolved Hide resolved
kit/Kit.cpp Outdated Show resolved Hide resolved
wsd/ClientSession.cpp Outdated Show resolved Hide resolved
@mmeeks
Copy link
Contributor

mmeeks commented Oct 7, 2024

It would be good to cleanup the ordering functionality at the same time though eg.

// FIXME: it's not that clear what good this does for us ...
// we process all previews in the same batch of rendering
void KitQueue::deprioritizePreviews()

Around thumbnailing slides - while ensuring that whatever algorithm we use doesn't create starvation problems.

Sven Göthel added 2 commits October 9, 2024 05:10
…const`

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Ic71b03747e57976ce4ac69a5f97a015d5d210648
As an attempt to increase responsiveness for client,
we only send invisible requested combined-tiles if IDLE.
IDLE is determined if socket poll timed out, i.e. nothing else to do for client.

Signed-off-by: Sven Göthel <[email protected]>
Change-Id: Ic71b03747e57976ce4ac69a5f97a015d5d210648
@sgothel sgothel force-pushed the private/sgothel/cool_9989_deprio_rendering_invisible_tiles branch from 2841348 to 3a3acc9 Compare October 9, 2024 03:33
@sgothel
Copy link
Author

sgothel commented Oct 9, 2024

It would be good to cleanup the ordering functionality at the same time though eg.

// FIXME: it's not that clear what good this does for us ... // we process all previews in the same batch of rendering void KitQueue::deprioritizePreviews()

Looking at it, it only pushes leading preview-tiles to the end of the queue
and you mentioned in the comment that a tile-req batch either only consist out of
such preview-tiles or none shall be included(?).

Our IDLE/invisible mechanism act above the produced 'popWholeTileQueue'
on the produced combined tiles, i.e. included this preview reordering.
Not sure if this is the discussion here.

Around thumbnailing slides - while ensuring that whatever algorithm we use doesn't create starvation problems.

Since out IDLE/invisible mechanism includes the preview-tile order, see above,
this should be well included in the solution.

@sgothel
Copy link
Author

sgothel commented Oct 9, 2024

Offered the optional Tile/TileCombined intersection test patch via
#10197

@sgothel
Copy link
Author

sgothel commented Oct 9, 2024

COOL 24.04 Cypress (desktop) — FAILURE:
Passed locally: make check-desktop spec=writer/invalidations_spec.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

perf: de-prioritize rendering non-visible tiles
2 participants