Re: RFC removing the XPrimitive2D (and related) UNO classes
Hi Tomaž, Tomaž Vajngerl wrote: > I'd call it a flaw in the dependency hierarchy, which means something is > located at a wrong place. > Not necessarily. Sometimes it's the lesser evil (and there's always other design constraints). Not saying this is the case here (haven't looked in any detail if/how this can be disentangled). > I know that this can be a "standard-issue" but mostly because a lot > of times fixing the hierarchy is a lot of work and moving things > around, but this doesn't mean breaking a circular dependency in such > a way should be taken lightly or is a good thing. > Yep. > As for UNO - to me UNO API is for making the functionality available > to extensions, macros, tests. If we use UNO just to circumvent a > circular dependency, how is that anything but a misuse of UNO API? > It's the standard component model for LibreOffice, for better or for worse. And it's fairly complete with stuff like factories, and more subtle things like thread apartements. So using UNO for a bit of fairly self-contained code, that can usefully be employed by extensions (this "it is not perfect yet, you cannot use it as-is" is a red herring really), and gives you a factory that breaks the dependency chain is IMO not a bad thing. Hand-rolling DLL loading to work around it would be. > If we extend the UNO API from the point were it is not required for > extension development, it is prone to be used for things that were > never meant to be used externally and also YAGNI. > There's some truth to that. But it's very hard to draw lines in the sand here; there's historically been extensions trying to integrate _very_ intricately even with document views. The discussion at hand is a different one though: should we deliberately _remove_ UNO API, despite there being no technical need, because we _think_ extension authors wouldn't/shouldn't use it? (the icing of the cake is of course doing that, and then noticing now you need to code something like the component factory yourself to get dependency inversion working) Cheers, -- Thorsten signature.asc Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: RFC removing the XPrimitive2D (and related) UNO classes
Hi, On Tue, Apr 14, 2020 at 1:38 PM Thorsten Behrens wrote: > Noel Grandin wrote: > > It seems to me that in quite a few places we are using UNO as a > > means of papering over circular dependencies. > > > Would call that papering over - ain't that quite standard-issue > dependency-inversion? > I'd call it a flaw in the dependency hierarchy, which means something is located at a wrong place. I know that this can be a "standard-issue" but mostly because a lot of times fixing the hierarchy is a lot of work and moving things around, but this doesn't mean breaking a circular dependency in such a way should be taken lightly or is a good thing. As for UNO - to me UNO API is for making the functionality available to extensions, macros, tests. If we use UNO just to circumvent a circular dependency, how is that anything but a misuse of UNO API? If we extend the UNO API from the point were it is not required for extension development, it is prone to be used for things that were never meant to be used externally and also YAGNI. > Cheers, > > -- Thorsten > Best regards, Tomaž ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: RFC removing the XPrimitive2D (and related) UNO classes
Hi Noel, Noel Grandin wrote: > That is kind of my point - here we have gratutious use of UNO, with > no real means of an extension using it, which is just making it > harder to optimise an important of our system. > The main point I'm disputing is the 'making it harder to optimise'. From the patch, nothing prevents you from granting direct, c++-level access to internal data, _and_ keeping UNO in place. So without arguing the merits of whether UNO is useful for drawinglayer/svg, you're mixing two unrelated changes into one patch. > (1) Unnecessary copying because of UNO shows up heavily in various places. > Mostly because we can't pass large complex data directly. > Problem of API design. We have examples of complex data structures wrapped in UNO interfaces (and direct, c++-level access to implementation if necessary for speed). > (2) Lots of UNO classes need to have their own Mutex object because > they can get called from multiple threads, so it is not just > SolarMutex. > For graphics stuff, that's a non sequitur - we can just grab SolarMutex on the outer layer. > (3) Even where UNO is not itself a perf problem, it introduces > indirection because it is much harder to figure out what code is > being called and who is calling it > That's mostly true for the heavily property-based interfaces for our document APIs, that then largely affect filter code. > (4) Touching UNO is an expensive exercise, involving attempted > analysis of external usage, awkardness introducing modifications > to API, etc, etc (and this coming from someone who __likes__ > UNO) > Yep, that's true. Cheers, -- Thorsten signature.asc Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: RFC removing the XPrimitive2D (and related) UNO classes
Noel Grandin wrote: > It seems to me that in quite a few places we are using UNO as a > means of papering over circular dependencies. > Would call that papering over - ain't that quite standard-issue dependency-inversion? Cheers, -- Thorsten signature.asc Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: RFC removing the XPrimitive2D (and related) UNO classes
On 2020/04/14 12:31 am, Thorsten Behrens wrote: Don't quite see the logic here - I'd hate to lose API, which in principle (if something's missing, bug report appreciated) allows external code to do cool things. The code removal in gerrit IMO is gratuitous, you could simply overload getDrawCommands et al with a c++-only API variant. That is kind of my point - here we have gratutious use of UNO, with no real means of an extension using it, which is just making it harder to optimise an important of our system. I like it much better - and if speed is of concern, one can then always dynamic_cast to an implementation class. I posit that UNO per se does not impose any performance penalties (modulo cost of abstraction if the API is crap, and perhaps for synchronisation - but for that, anything graphics will have SolarMutex anyway below the first API layer). (1) Unnecessary copying because of UNO shows up heavily in various places. Mostly because we can't pass large complex data directly. (2) Lots of UNO classes need to have their own Mutex object because they can get called from multiple threads, so it is not just SolarMutex. (3) Even where UNO is not itself a perf problem, it introduces indirection because it is much harder to figure out what code is being called and who is calling it (4) Touching UNO is an expensive exercise, involving attempted analysis of external usage, awkardness introducing modifications to API, etc, etc (and this coming from someone who __likes__ UNO) ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: RFC removing the XPrimitive2D (and related) UNO classes
On 2020/04/13 9:34 pm, Tomaž Vajngerl wrote: Yeah, that looks great to me, but I don't like that dynamic / static loading of svgio library (like we already do in I'm just exposing an already-existing dependency here - currently the dynamic loading happens inside the UNO layer. It seems to me that in quite a few places we are using UNO as a means of papering over circular dependencies. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: RFC removing the XPrimitive2D (and related) UNO classes
Hi Tomaž, hi Noel, Tomaž Vajngerl wrote: > On Mon, Apr 13, 2020 at 7:46 PM Noel Grandin wrote: > > The benefit is that it becomes easier to optimise the copying and moving > > around of this stuff if it is C++ layers all the way down, with no UNO > > stuck in the middle of it. > > > Don't quite see the logic here - I'd hate to lose API, which in principle (if something's missing, bug report appreciated) allows external code to do cool things. The code removal in gerrit IMO is gratuitous, you could simply overload getDrawCommands et al with a c++-only API variant. > Yeah, that looks great to me, but I don't like that dynamic / static > loading of svgio library (like we already do in graphicfilter). Ideally > svgio shouldn't need to depend on vcl - it just creates the primitives from > the svg file, so vcl could just import svgio normally. > Which is another nice side effect, that with UNO you get the necessary dependency inversion for free.. > Anyway, an alternative to this would also be to create a > XPrimtiive2DContainer UNO interface, which would allow to "transport" the > Primitive2DContainer unmodified and wouldn't require that we convert, only > on demand convert that to Sequence. Not sure if this solution > is any better... > I like it much better - and if speed is of concern, one can then always dynamic_cast to an implementation class. I posit that UNO per se does not impose any performance penalties (modulo cost of abstraction if the API is crap, and perhaps for synchronisation - but for that, anything graphics will have SolarMutex anyway below the first API layer). Cheers, -- Thorsten signature.asc Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: RFC removing the XPrimitive2D (and related) UNO classes
Hi Noel, On Mon, Apr 13, 2020 at 7:46 PM Noel Grandin wrote: > Hi > > I notice when performance tuning that our drawinglayer stuff spends quite > some time converting back and forth between XPrimitive2D and BasePrimitive > stuff and copying sequence->vector and vector->sequence. > > Now as far as I can see (analysis below), the stuff exposed in our UNO > layer is not usable from extensions because it doesn't tie into anything > useful, so nothing should change as far as that goes. > > The benefit is that it becomes easier to optimise the copying and moving > around of this stuff if it is C++ layers all the way down, with no UNO > stuck in the middle of it. > > First stage of this change here: > https://gerrit.libreoffice.org/c/core/+/92107 > > Yeah, that looks great to me, but I don't like that dynamic / static loading of svgio library (like we already do in graphicfilter). Ideally svgio shouldn't need to depend on vcl - it just creates the primitives from the svg file, so vcl could just import svgio normally. For that also drawinglayer needs to be free from vcl dependency, so it is not a simple thing to do. I'm experimenting with this in feature/drawinglayercore branch [1], where I made a drawinglayercore library (like svx and svxcore, which is not ideal but necessary for simplicity), which will only have the core drawinglayer stuff and basic primitives and basegfx as the dependency. [1] https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/drawinglayercore=3abc1847fb81c358737e1d4443f3e6bf83c7691d Anyway, an alternative to this would also be to create a XPrimtiive2DContainer UNO interface, which would allow to "transport" the Primitive2DContainer unmodified and wouldn't require that we convert, only on demand convert that to Sequence. Not sure if this solution is any better... Regards, Noel > > Regards, Tomaž ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
