Poor Performance of Text Drawing on Linux (Easily Fixed)

[JoshParnell]

Hello! Thank you Laurent for this work of code art It is a truly beautiful library.

A small performance issue that has recently bitten me (and is basically the only SFML issue to ever have done so!) : text rendering suffers major performance issues on Linux due to the implementation of Window::getSize.  Window does not cache it's own size, so it needs to go to the OS every time getSize is called, and it is called during text rendering.  On Windows apparently this is no problem, (I have never had an issue with it) I guess GetClientRect is fast.  On Linux, at least the flavor I am running, I see a large performance impact when large amounts of text is being drawn.  XGetWindowAttributes appears to not be anywhere near as snappy as GetClientRect.  The majority of the time when I halt my app in gdb, I am seeing that it is waiting on the XGetWindowAttributes syscall to respond.

It is easily fixed via a thin caching layer. If I understand the architecture correctly, we can store the window size once at creation, and then update it via the resize message.  Then we can return cached size in the call to getSize.  This minor change solved the problem for me, and it made the difference between my game running at 70 mspf without the opt, down to 16 mspf with it.  With this change my Linux port is as responsive as Windows

I'm sure you don't want to muddy your architecture with caching, but we really need to in this case - I don't think it's necessary to go to a syscall in what would seem to a client to be a thin getter.  And, as I said, it is a serious performance concern, at least for Linux.

Thanks again for your excellent work!

[Laurent]

Window does not cache it's own size, so it needs to go to the OS every time getSize is called, and it is called during text rendering

What I don't understand is, why is it called during text rendering? Is it called by SFML itself or by you? And only for text??

[The Hatchet]

He may be calling getSize when rendering text to make sure that his text is being displayed in the same spot relative to the current window size/shape is what.  I'm thinking game HUD elements that are on the right or bottom portions of the screen you'd have to do this to keep them in the same spots during resolution change.

[Laurent]

This kind of calculation must be done relatively to the view, not to the window.

[JoshParnell]

Hi Laurent, you're right, it is inside the calculation of the view.  It is from SFML, I am simply calling window.Draw(textBuffer).  Is this not the correct way to draw a textbuffer to a renderwindow?

I reverted the change to get you the callstack:

0x00132416 in __kernel_vsyscall ()
0x00687690 in __GI___poll (fds=0xbfffdfe8, nfds=1, timeout=-1)
0x0292f120 in ?? ()
0x02930a60 in ?? ()
0x02930d17 in xcb_wait_for_reply ()
0x0088d682 in _XReply ()
0x008734d8 in _XGetWindowAttributes ()
0x00873625 in XGetWindowAttributes ()
0x0013dd74 in sf::priv::WindowImplX11::getSize() const ()
0x00139f12 in sf::Window::getSize() const ()
0x00b3d5fa in sf::RenderWindow::getSize() const ()
0x00b3c90b in sf::RenderTarget::getViewport(sf::View const&) const ()
0x00b3ce2c in sf::RenderTarget::applyCurrentView() ()
0x00b3d4e0 in sf::RenderTarget::draw(sf::Vertex const*, unsigned int, sf::PrimitiveType, sf::RenderStates const&) ()
0x00b46d24 in sf::VertexArray::draw(sf::RenderTarget&, sf::RenderStates) const ()
0x00b3cc3e in sf::RenderTarget::draw(sf::Drawable const&, sf::RenderStates const&) ()
0x00b458bd in sf::Text::draw(sf::RenderTarget&, sf::RenderStates) const ()
0x00b3cc3e in sf::RenderTarget::draw(sf::Drawable const&, sf::RenderStates const&) ()

Seems odd to me that the viewport is having to query the window's size?

[JoshParnell]

Ah! Yes, the rendertarget computes viewport on the fly, hence the size is never actually cached by the viewport:

IntRect RenderTarget::getViewport(const View& view) const
{
    float width  = static_cast<float>(getSize().x);
    float height = static_cast<float>(getSize().y);
    const FloatRect& viewport = view.getViewport();

    return IntRect(static_cast<int>(0.5f + width  * viewport.left),
                   static_cast<int>(0.5f + height * viewport.top),
                   static_cast<int>(width  * viewport.width),
                   static_cast<int>(height * viewport.height));
}
 

getSize() is the offender (twice!)

[Laurent]

I see. So it's not just for text, but for everything else as well. And yes, this could be optimized.

Don't hesitate to open a task in the tracker, so that I don't forget it

[texus]

I could use this optimization too, it would make some of my code a lot faster without having to cache the size myself.

Changing the getSize function to the following would be the easiest way to fix it.

Vector2u WindowImplX11::getSize() const
{
    if (m_previousSize.x == -1 && m_previousSize.y == -1)
    {
        XWindowAttributes attributes;
        XGetWindowAttributes(m_display, m_window, &attributes);
        return Vector2u(attributes.width, attributes.height);
    }
    else
    {
        return Vector2u(m_previousSize);
    }
}

Or do you want even better caching so that all the function needs to do is just "return m_size;" ?
Then another member should be added because m_previousSize can't be used to store the correct size from the beginning or you might not get a resize event when the window is created.

I could create a pull request if you want, I just wanted to know which way you find better.

[Laurent]

I don't know, I have to look at the implementation. But that would probably be handled at the generic level (sf::WindowImpl base class), so that the getSize virtual function can be removed. I also have to check which functions could work the same way, if any.

So just create a ticket, and I'll work on that later (but soon ).

[JoshParnell]

I don't know, I have to look at the implementation. But that would probably be handled at the generic level (sf::WindowImpl base class), so that the getSize virtual function can be removed. I also have to check which functions could work the same way, if any.

So just create a ticket, and I'll work on that later (but soon ).

Agreed, IMO should be handled at generic level so all implementations benefit, even if only slightly for Windows (have not tested on Mac yet, it's possible that this could be significant there as well).

Will make a ticket sometime when I get a github account

@texus : No need to worry about window creation, we can just initialize the cached value(s) inside the window's initialize() function! It's exactly what that function is for

getPosition I guess should do this as well, although probably less-commonly called, still a client would expect it to be a lightweight function.  I believe those are the only two that pose a threat currently.

[texus]

Will make a ticket sometime when I get a github account

I've alreade made one, so you don't have to do this anymore: https://github.com/SFML/SFML/issues/405

[JoshParnell]

Will make a ticket sometime when I get a github account

I've alreade made one, so you don't have to do this anymore: https://github.com/SFML/SFML/issues/405

Beautiful, thank you 

[Laurent]

I started to implement the generci solution and quickly got stuck on the constructor which takes a WindowHandle; in this case there's no way to know the initial size of the window other than asking the OS. So the final solution will probably have to be handled in each implementation.

[JoshParnell]

I started to implement the generci solution and quickly got stuck on the constructor which takes a WindowHandle; in this case there's no way to know the initial size of the window other than asking the OS. So the final solution will probably have to be handled in each implementation.

Hey Laurent,

What's wrong with asking the OS in initialize()?  It is only one time, so it should not be a big concern...as long as we aren't asking the OS every time...unless I have missed something.  It is called from both creation paths, as far as I can tell.

I stuck a

m_size = m_impl->getSize();

In initialize().  It seems to work for my purposes, at least

[Laurent]

Ah, right.

I did the modification. Enjoy