Memorize the bit depth for drawable.

kc- commented 5 years ago

For context, please see https://github.com/McCLIM/McCLIM/issues/637 -- the performance of some implementations of X (in this case, XQuartz) justifies this memoization.

kc- commented 5 years ago

@scymtym I put in the aesthetic changes we spoke of. Unfortunately, def-clx-class uses structures, so I couldn't use a :documentation slot option, which I would have preferred.

scymtym commented 5 years ago

LGTM.

JMC-design commented 5 years ago

I'm not sure I see the point of this commit. WITH-GEOMETRY keeps track of drawable-depth. Was there an actual problem with the existing cache? Requests like that should get bundled properly into the queue and not cause extra requests. If the problem lies there it needs to be corrected for everything else that uses it. The mcclim issue does not clarify this commit, why would drawable-depth even be called that often?

kc- commented 5 years ago

@JMC-design thank you for your comment. I absolutely do not pretend to know my way around CLX very well, but I can expand the context on the McCLIM issue. Here's a profile using XQuartz , and on Linux/X11, you can tell the factor of around 20 between the two X implementations.

why would drawable-depth even be called that often

For the pattern drawing (the test code in https://github.com/McCLIM/McCLIM/issues/637), the call chain goes roughly like this:

[draw-pattern] -> [draw-rectangle] -> medium-draw-rectangle* -> with-clx-graphics -> design-gcontext -> compute-rgb-image, in which the depth is queried.

If your point is "don't do that, then!", fair enough -- maybe you can propose a superior fix for the performance issue with this, other than caching that value in user code.

JMC-design commented 5 years ago

Dont do that. Seriously though the gc code there doesn't seem unified and you seem to be wasting cycles checking and creating new gcs.
That profile doesn't real show why its caught up in draw rectangles. My concern here is a bandaid solution possibly covering up the real problem just to get an implementation of clim to work performantly. The clx attribute and context cache shouldn't be inflating times x20, nor should we need two things accomplishing the same goal.

kc- commented 5 years ago

The factor of 20 is the macOS' XQuartz tax, I don't know why it is that much slower than on linux.

Maybe we can discuss this first: do you agree that asking CLX for an immutable value (the drawable depth) could do without the network connection every time? In that context, I don't think this commit is the wrong thing to change.

Maybe others can weigh in with their opinions on this.

Edit:

That profile doesn't real show why its caught up in draw rectangles.

Well the code draws 26^2 = 676 copies of that 2x2 pixel pattern on a canvas. The lions share of time is spent at the point before the pixmap that holds a copy of the pattern is created, in the call to xlib:drawable-depth. That's where this PR came from, and you can see how it improved the time spent by comparing the results of the (time ...) around the repaint bodies -- first and last comment in the McCLIM issue thread.

Now I don't know about the real-life use case that that is, but I can only imagine that it involves more than drawing one predefined pattern on an area known at compile time.

What do you suggest? Where might one find some reference material that clarifies the best use of things like gcontext creation and so on? Thank you for any hints.

JMC-design commented 5 years ago

The manual mentions reusing gcontexts is usually more efficient.

Your question is the wrong one, considering that drawable-depth should already be cached and shouldn't require a network connection.

So like I said in the beginning, if the problem lies with the existing cache then that needs to be investigated since it caches all window/drawable attributes, and not covered up by keeping track of drawable depth in a second manner to fix performance of one product, on one operating system.

Perhaps the issue is competition for the main thread on macos since certain things have to be run on it?

p.s. that profile still doesn't show what you are saying it shows, perhaps there was a problem in the conversion process, quite wasteful, of turning your pattern into something to be displayed. I mean, has anybody else had any of the problems you mentioned? Besides people who were running the clos version?

kc- commented 5 years ago

Which question is the wrong one, "What do you suggest?" or "Where can I find reference material?" ?

I already said that I'm not intimately familiar with CLX. You're uncomfortable with this patch, and I acknowledged that -- maybe at this point you could be a little more forthcoming with a characterization of what The Right Way would be, please.

Is xlib:drawable-depth supposed to access this cache you mentioned, or is it the wrong accessor for that purpose? Is WINDOW-ATTRIBUTES the cache you mean, and should user code like mcclim access that?

that profile still doesn't show what you are saying it shows

I don't understand what you mean. I didn't put xlib:drawable-depth in the profile list because at that point I was still looking for the cause. Later, I localized it more narrowly -- to the call to xlib:drawable-depth inside of the macroexpansion of [code you probably don't care about]-- and after this patch the performance is back to what it should have been. That is what I am saying it shows, even without an explicit mention of xlib in the profile table. At what point do we disagree?

JMC-design commented 5 years ago

drawable-depth should already be cached and shouldn't require a network connection

kc- commented 5 years ago

@JMC-design thank you for your comment.

JMC-design commented 5 years ago

Just to make sure you understand the last comment. It is not an agreement with you, it is the exact same words I said before because I dislike repeating myself, but here I go.

WITH-GEOMETRY inside DRAWABLE-DEPTH already caches the depth information.

If this is affecting times it needs to be invesigated and not covered up with a bandaid.

kc- commented 5 years ago

I repeated myself verbatim because you did the same thing. I tried asking for relevant information but you don't seem to want to follow up, which is fine of course.

JMC-design commented 5 years ago

How much more relevant do you need than WITH-GEOMETRY already caches DRAWABLE-DEPTH, you shouldn't need to cache it again! Are you understanding you are duplicating behaviour to compensate for something only you have seen and haven't provided proof of and might not even be the fault of CLX!

dkochmanski commented 5 years ago

If I understand correctly with-state caches results until we leave its dynamic extent while this cache change makes it cache the depth inside drawable object (so it is persistent across different calls). It is not very obvious from the client perspective, i.e McCLIM creates drawables which are reused and with-state can't be instantiated globally, so there is certainly something into it.

That said it could be easily mitigated with the plist extension hook from the client code (I.e McCLIM's backend specification). Something in line of:

(defun cached-drawable-depth (drawable)
  (or (getf (xlib:drawable-plist drawable) :cached-depth)
        (setf (getf (xlib:drawable-plist drawable) :cached-depth)
                 (xlib:drawable-depth drawable))))

@JMC-design all dissatisfaction should be directed towards me, I gave a green light for this PR without knowing about with-state (I should have known better).

OTOH please note, that with-state is not feasible for McCLIM model and given that drawable-depth query is very slow on OSX it is a valid solution. I rather agree though that such caching should be done on client side; but I don't have very strong opinion about that.

@kc- do you agree with reverting this change and adding this "cache" on McCLIM side?

JMC-design commented 5 years ago

I do not know how mcclim deals with events, multiple clients,etc.., but if you use the windows plist you need to make sure you never try to access the plist on a window returned by an event.

My main concern is still what is causing this slowdown? 1 call to drawable depth should not inflate times like that. You could add an extra 20 network calls per cycle and it wouldn't come close to being that slow. So what is going on here?

another possibility is rehashing of the xid hashtable. I have also noticed some issues with deletion of xids sometimes not happening and then getting huge slowdowns when instantiating thousands of items. It's on my todo list when I start dealing with thousands of objects not in testing.

kc- commented 5 years ago

@dkochmanski Of course I don't disagree with moving this cache to mcclim.

@JMC-design

How much more relevant do you need than WITH-GEOMETRY already caches DRAWABLE-DEPTH, you shouldn't need to cache it again! Are you understanding you are duplicating behaviour to compensate for something only you have seen and haven't provided proof of and might not even be the fault of CLX!

You said this repeatedly, and I don't disagree with you either, in the sense that CLX has a caching facility and this should be preferred over tacking something extra on. Of course! But:

the *window-attributes* cache is created in with-state, not with-geometry,
the code paths from https://github.com/McCLIM/McCLIM/issues/637 don't use either with-state or with-geometry -- in fact the whole of mcclim doesn't. When I inquired about *window-attributes* and this cache, I meant "Is client code supposed to wrap things in with-state or is that used in clx somewhere?"
it isn't just me that has seen this behaviour: Are you understanding that I'm not the issue reporter of https://github.com/McCLIM/McCLIM/issues/637 ?
I provided links to relevant functions and macros (of the code path used for the McCLIM issue), you still claim that the evidence doesn't support my conclusion
In my comment here from two days ago I layed out what lead to identifying xlib:drawable-depth as the main time sink -- I asked you where the misunderstanding might be -- you ignored the question.

I'm trying to work with you, but you're not making it very easy. Why are you so hostile?

kc- commented 5 years ago

Let's start over, okay?

My main concern is still what is causing this slowdown?

I don't know. What I can offer as a conversation piece is a more explicit comparison of the timing without and with this changeset (these are the times profiled for running run-drawtest from the McCLIM issue report)

Without:

  seconds  |     gc     |   consed   |  calls |  sec/call  |  name  
---------------------------------------------------------
    16.257 |      0.000 |    259,600 |  1,369 |   0.011875 | XLIB:DRAWABLE-DEPTH
     0.116 |      0.000 |  3,671,472 |  1,352 |   0.000086 | CLIM-CLX::DESIGN-GCONTEXT
     0.089 |      0.000 |  7,953,696 |  2,035 |   0.000044 | MEDIUM-DRAW-RECTANGLE*
     0.016 |      0.000 |  3,076,896 |  9,665 |   0.000002 | TRANSFORM-REGION
     0.009 |      0.000 |    754,288 |  1,371 |   0.000007 | CLIM-CLX::MEDIUM-GCONTEXT
     0.009 |      0.000 |     32,768 |  1,535 |   0.000006 | COMPOSE-TRANSFORMATIONS
     0.008 |      0.000 |    163,680 |  2,704 |   0.000003 | XLIB:CREATE-IMAGE
     0.007 |      0.000 |          0 |  6,137 |   0.000001 | MEDIUM-INK
     0.007 |      0.000 |          0 |  4,056 |   0.000002 | XLIB:FREE-GCONTEXT
     0.003 |      0.000 |          0 |  2,880 |   0.000001 | SHEET-NATIVE-TRANSFORMATION
     0.003 |      0.000 |          0 |  4,775 |   0.000001 | MEDIUM-LINE-STYLE
     0.002 |      0.000 |    524,528 |    676 |   0.000003 | DRAW-PATTERN*
     0.002 |      0.000 |          0 |    680 |   0.000003 | DRAW-RECTANGLE*
     0.001 |      0.000 |          0 |     16 |   0.000055 | MEDIUM-DRAW-POLYGON*
     0.000 |      0.000 |          0 |    980 |   0.000000 | RECTANGLE-EDGES*
     0.000 |      0.000 |          0 |    676 |   0.000000 | DRAW-DESIGN
---------------------------------------------------------
    16.529 |      0.000 | 16,436,928 | 40,907 |            | Total

With:

  seconds  |     gc     |   consed   |  calls |  sec/call  |  name  
---------------------------------------------------------
     0.053 |      0.000 |  4,453,360 |  1,352 |   0.000039 | CLIM-CLX::DESIGN-GCONTEXT
     0.035 |      0.000 |  8,148,208 |  2,035 |   0.000017 | MEDIUM-DRAW-RECTANGLE*
     0.021 |      0.000 |  1,932,448 |  9,665 |   0.000002 | TRANSFORM-REGION
     0.018 |      0.000 |          0 |  1,369 |   0.000013 | XLIB:DRAWABLE-DEPTH
     0.009 |      0.000 |     32,768 |  2,704 |   0.000003 | XLIB:CREATE-IMAGE
     0.005 |      0.000 |          0 |  4,775 |   0.000001 | MEDIUM-LINE-STYLE
     0.004 |      0.000 |          0 |  4,056 |   0.000001 | XLIB:FREE-GCONTEXT
     0.004 |      0.000 |     32,752 |  1,555 |   0.000002 | COMPOSE-TRANSFORMATIONS
     0.003 |      0.000 |          0 |  6,137 |   0.000001 | MEDIUM-INK
     0.001 |      0.000 |          0 |  3,183 |   0.000000 | SHEET-NATIVE-TRANSFORMATION
     0.001 |      0.000 |     32,768 |     16 |   0.000055 | MEDIUM-DRAW-POLYGON*
     0.000 |      0.000 |          0 |  1,030 |   0.000000 | RECTANGLE-EDGES*
     0.000 |      0.000 |          0 |    676 |   0.000000 | DRAW-DESIGN
     0.000 |      0.000 |          0 |    680 |   0.000000 | DRAW-RECTANGLE*
     0.000 |      0.000 |    654,976 |    676 |   0.000000 | DRAW-PATTERN*
     0.000 |      0.000 |  1,113,040 |  1,371 |   0.000000 | CLIM-CLX::MEDIUM-GCONTEXT
---------------------------------------------------------
     0.153 |      0.000 | 16,400,320 | 41,280 |            | Total

These times are with XQuartz 2.7.11 . The linux X11 doesn't seem to share this problem.

dkochmanski commented 5 years ago

Could you share linux results for comparison?

On August 21, 2019 7:31:04 PM GMT+02:00, "Christoph Keßler" notifications@github.com wrote:

Let's start over, okay?

My main concern is still what is causing this slowdown?

I don't know. What I can offer as a conversation piece is a more explicit comparison of the timing without and with this changeset (these are the times profiled for running run-drawtest from the McCLIM issue report)

Without:

 seconds  |     gc     |   consed   |  calls |  sec/call  |  name  
---------------------------------------------------------
16.257 |      0.000 |    259,600 |  1,369 |   0.011875 |
XLIB:DRAWABLE-DEPTH
0.116 |      0.000 |  3,671,472 |  1,352 |   0.000086 |
CLIM-CLX::DESIGN-GCONTEXT
0.089 |      0.000 |  7,953,696 |  2,035 |   0.000044 |
MEDIUM-DRAW-RECTANGLE*
0.016 |      0.000 |  3,076,896 |  9,665 |   0.000002 |
TRANSFORM-REGION
0.009 |      0.000 |    754,288 |  1,371 |   0.000007 |
CLIM-CLX::MEDIUM-GCONTEXT
0.009 |      0.000 |     32,768 |  1,535 |   0.000006 |
COMPOSE-TRANSFORMATIONS
0.008 |      0.000 |    163,680 |  2,704 |   0.000003 |
XLIB:CREATE-IMAGE
    0.007 |      0.000 |          0 |  6,137 |   0.000001 | MEDIUM-INK
0.007 |      0.000 |          0 |  4,056 |   0.000002 |
XLIB:FREE-GCONTEXT
0.003 |      0.000 |          0 |  2,880 |   0.000001 |
SHEET-NATIVE-TRANSFORMATION
0.003 |      0.000 |          0 |  4,775 |   0.000001 |
MEDIUM-LINE-STYLE
 0.002 |      0.000 |    524,528 |    676 |   0.000003 | DRAW-PATTERN*
0.002 |      0.000 |          0 |    680 |   0.000003 | DRAW-RECTANGLE*
0.001 |      0.000 |          0 |     16 |   0.000055 |
MEDIUM-DRAW-POLYGON*
0.000 |      0.000 |          0 |    980 |   0.000000 |
RECTANGLE-EDGES*
   0.000 |      0.000 |          0 |    676 |   0.000000 | DRAW-DESIGN
---------------------------------------------------------
   16.529 |      0.000 | 16,436,928 | 40,907 |            | Total

With:

 seconds  |     gc     |   consed   |  calls |  sec/call  |  name  
---------------------------------------------------------
0.053 |      0.000 |  4,453,360 |  1,352 |   0.000039 |
CLIM-CLX::DESIGN-GCONTEXT
0.035 |      0.000 |  8,148,208 |  2,035 |   0.000017 |
MEDIUM-DRAW-RECTANGLE*
0.021 |      0.000 |  1,932,448 |  9,665 |   0.000002 |
TRANSFORM-REGION
0.018 |      0.000 |          0 |  1,369 |   0.000013 |
XLIB:DRAWABLE-DEPTH
0.009 |      0.000 |     32,768 |  2,704 |   0.000003 |
XLIB:CREATE-IMAGE
0.005 |      0.000 |          0 |  4,775 |   0.000001 |
MEDIUM-LINE-STYLE
0.004 |      0.000 |          0 |  4,056 |   0.000001 |
XLIB:FREE-GCONTEXT
0.004 |      0.000 |     32,752 |  1,555 |   0.000002 |
COMPOSE-TRANSFORMATIONS
    0.003 |      0.000 |          0 |  6,137 |   0.000001 | MEDIUM-INK
0.001 |      0.000 |          0 |  3,183 |   0.000000 |
SHEET-NATIVE-TRANSFORMATION
0.001 |      0.000 |     32,768 |     16 |   0.000055 |
MEDIUM-DRAW-POLYGON*
0.000 |      0.000 |          0 |  1,030 |   0.000000 |
RECTANGLE-EDGES*
   0.000 |      0.000 |          0 |    676 |   0.000000 | DRAW-DESIGN
0.000 |      0.000 |          0 |    680 |   0.000000 | DRAW-RECTANGLE*
 0.000 |      0.000 |    654,976 |    676 |   0.000000 | DRAW-PATTERN*
0.000 |      0.000 |  1,113,040 |  1,371 |   0.000000 |
CLIM-CLX::MEDIUM-GCONTEXT
---------------------------------------------------------
    0.153 |      0.000 | 16,400,320 | 41,280 |            | Total

These times are with XQuartz 2.7.11 . The linux X11 doesn't seem to share this problem.

-- Wysłane za pomocą K-9 Mail.

kc- commented 5 years ago

I'm sorry, I don't have any linux installations ready at the moment. My third comment in this thread has links to a profile comparison (in those profiles, drawable-depth was not recorded explicitly, but is part of the "medium-draw-rectangle* entry).

In case you get a chance to check on your machine, here's the setup: (sb-profile:profile draw-pattern* medium-draw-rectangle* draw-rectangle* transform-region compose-transformations rectangle-edges* draw-design clim-internals::sheet-native-transformation clim-internals::medium-draw-polygon* clim-clx::medium-gcontext clim-clx::medium-line-style clim-clx::medium-ink xlib:create-image xlib:drawable-depth xlib:free-gcontext clim-clx::design-gcontext)

sharplispers / clx

Memorize the bit depth for drawable. #146