You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flex.apache.org by Maurice Amsellem <ma...@systar.com> on 2013/11/03 19:00:41 UTC
Need advice on patch validation
Hi Team,
Benoit Wiart has raised the following JIRA : https://issues.apache.org/jira/browse/FLEX-33865 (ConstraintLayout / LayoutElementHelper are memory inefficient and slow)
He also proposed a patch to optimize the performance and reduce memory allocations.
On behalf on the Apache Flex team, I would like to thank him for the time he spent on that.
This is my almost first patch validation and would like your advice:
1) The patch adds an optional arg to an existing method that returns an Array of 2 elements. The purpose of this arg is to preallocate an Array, and pass it to successive calls to avoid memory allocations.
This certainly decreases the memory allocations, but at the same time makes the code less readable, and may also have an impact on other calls when the second arg is not used.
2) Patch introduces "inlined" code for trimming spaces, again to avoid unnecessary allocs when calling utils functions, which makes the code more verbose (final function is 2.5 longer than original).
3) Patch speed optimization is x4 in worst case (constraint layout parsing) and memory allocation is 4x lower, when calling the function 10,000 times. But I am not sure what the overall performance gain is.
So my question is :
What should be the criteria for accepting an optimization proposition ?
- Code readability / maintenability vs. performance optimization
- local performance gain vs. overall performance gain?
- Optimization scope (that is, functions that are not often used)
Can we also accept part of an optimization only ?
Regards,
Maurice
RE: Need advice on patch validation
Posted by Maurice Amsellem <ma...@systar.com>.
>My general opinion is that we may choose to compromise code readability a little only when the optimization is really worth it, especially for public method >signatures, because otherwise, applying the same logic extensively, we could end up with a SDK codebase very difficult to read, debug and improve. This >readability-performance tradeoff may be pushed towards performance when required in method implementations (e.g. inlining simple method calls, reusing >objects to reduce memory allocations, etc.)
I am in total agreement with this. That's also what I said in the ticket.
FYI, I did more tests to determine the specific improvements of each part:
Replacement of split/replace by indexOf to detect simple case: 10%
Array recycling: 1% (in the GC)
Maurice
-----Message d'origine-----
De : Cosma Colanicchia [mailto:cosmacol@gmail.com]
Envoyé : lundi 4 novembre 2013 09:33
À : Apache Flex Developers ML
Objet : Re: Need advice on patch validation
Thanks to Benoit, I can see the effort put into these patches.
However, in this case, I agree in general with the use of indexOf instead of regexp, and to inlining these methods (if it can’t be done by the compiler, see next). The additional array argument could also be ok as we are talking about an Helper class, I see its public methods as an extension to the “helped” class, and not as a part of the “public API”. If this is not the general position, we could consider reaching the same result in some other way? For example, recycling static member array variables in the helper class to avoid reallocations (I think another performance patch related to object deserialization was adopting a similar pattern).
Finally, as a side note, I read that Falcon is able to automatically inline method calls, optimizing the byte code at compile time (see [1] and [2] for example, at least it should be implemented in ASC2, don’t known if it also part of the donated Falcon codebase). If this is true, maybe we should give up this kind of manual optimization if we are going to switch to Falcon as the default compiler in the near future, or try to adhere to the requirements for this optimization to happen.
(Just my two cents)
[1] http://renaun.com/blog/2012/09/using-the-new-inline-metadata-in-asc2/
[2] http://www.bytearray.org/?p=4789
--
Cosma
2013/11/3 Maurice Amsellem <ma...@systar.com>
> Thanks Justin
> ________________________________________
> De : Justin Mclean [justin@classsoftware.com]
> Envoyé : dimanche 3 novembre 2013 22:26
> À : dev@flex.apache.org
> Objet : Re: Need advice on patch validation
>
> Hi,
>
> > What should be the criteria for accepting an optimization proposition ?
> Does it improve significantly improve performance in real use cases for
> the majority of users would be at the tip of my list. Calling a method
> 10,000 times is sometimes not a real use case.
>
> > Can we also accept part of an optimization only ?
> Of course that's the whole point of submitting patches, commit then review
> etc etc
>
> Thanks,
> Justin
>
RE: Need advice on patch validation
Posted by Maurice Amsellem <ma...@systar.com>.
FYI, I spent most of my Sunday reviewing the issue...
Maurice
-----Message d'origine-----
De : Cosma Colanicchia [mailto:cosmacol@gmail.com]
Envoyé : lundi 4 novembre 2013 09:40
À : Apache Flex Developers ML
Objet : Re: Need advice on patch validation
... and also thanks to Justin for the extensive review work :)
2013/11/4 Cosma Colanicchia <co...@gmail.com>
> Thanks to Benoit, I can see the effort put into these patches.
>
> My general opinion is that we may choose to compromise code
> readability a little only when the optimization is really worth it,
> especially for public method signatures, because otherwise, applying
> the same logic extensively, we could end up with a SDK codebase very
> difficult to read, debug and improve. This readability-performance
> tradeoff may be pushed towards performance when required in method
> implementations (e.g. inlining simple method calls, reusing objects to
> reduce memory allocations, etc.)
>
> However, in this case, I agree in general with the use of indexOf
> instead of regexp, and to inlining these methods (if it can’t be done
> by the compiler, see next). The additional array argument could also
> be ok as we are talking about an Helper class, I see its public
> methods as an extension to the “helped” class, and not as a part of
> the “public API”. If this is not the general position, we could
> consider reaching the same result in some other way? For example,
> recycling static member array variables in the helper class to avoid
> reallocations (I think another performance patch related to object deserialization was adopting a similar pattern).
>
> Finally, as a side note, I read that Falcon is able to automatically
> inline method calls, optimizing the byte code at compile time (see [1]
> and [2] for example, at least it should be implemented in ASC2, don’t
> known if it also part of the donated Falcon codebase). If this is
> true, maybe we should give up this kind of manual optimization if we
> are going to switch to Falcon as the default compiler in the near
> future, or try to adhere to the requirements for this optimization to happen.
>
>
> (Just my two cents)
>
> [1]
> http://renaun.com/blog/2012/09/using-the-new-inline-metadata-in-asc2/
> [2] http://www.bytearray.org/?p=4789
>
> --
> Cosma
>
>
> 2013/11/3 Maurice Amsellem <ma...@systar.com>
>
>> Thanks Justin
>> ________________________________________
>> De : Justin Mclean [justin@classsoftware.com] Envoyé : dimanche 3
>> novembre 2013 22:26 À : dev@flex.apache.org Objet : Re: Need advice
>> on patch validation
>>
>> Hi,
>>
>> > What should be the criteria for accepting an optimization proposition ?
>> Does it improve significantly improve performance in real use cases
>> for the majority of users would be at the tip of my list. Calling a
>> method
>> 10,000 times is sometimes not a real use case.
>>
>> > Can we also accept part of an optimization only ?
>> Of course that's the whole point of submitting patches, commit then
>> review etc etc
>>
>> Thanks,
>> Justin
>>
>
>
Re: Need advice on patch validation
Posted by Cosma Colanicchia <co...@gmail.com>.
and obviosly to Maurice :D
2013/11/4 Cosma Colanicchia <co...@gmail.com>
> ... and also thanks to Justin for the extensive review work :)
>
>
> 2013/11/4 Cosma Colanicchia <co...@gmail.com>
>
>> Thanks to Benoit, I can see the effort put into these patches.
>>
>> My general opinion is that we may choose to compromise code readability a
>> little only when the optimization is really worth it, especially for
>> public method signatures, because otherwise, applying the same logic
>> extensively, we could end up with a SDK codebase very difficult to read,
>> debug and improve. This readability-performance tradeoff may be pushed
>> towards performance when required in method implementations (e.g. inlining
>> simple method calls, reusing objects to reduce memory allocations, etc.)
>>
>> However, in this case, I agree in general with the use of indexOf instead
>> of regexp, and to inlining these methods (if it can’t be done by the
>> compiler, see next). The additional array argument could also be ok as we
>> are talking about an Helper class, I see its public methods as an extension
>> to the “helped” class, and not as a part of the “public API”. If this is
>> not the general position, we could consider reaching the same result in
>> some other way? For example, recycling static member array variables in the
>> helper class to avoid reallocations (I think another performance patch
>> related to object deserialization was adopting a similar pattern).
>>
>> Finally, as a side note, I read that Falcon is able
>> to automatically inline method calls, optimizing the byte code at compile
>> time (see [1] and [2] for example, at least it should be implemented in
>> ASC2, don’t known if it also part of the donated Falcon codebase). If this
>> is true, maybe we should give up this kind of manual optimization if we are
>> going to switch to Falcon as the default compiler in the near future, or
>> try to adhere to the requirements for this optimization to happen.
>>
>>
>> (Just my two cents)
>>
>> [1] http://renaun.com/blog/2012/09/using-the-new-inline-metadata-in-asc2/
>> [2] http://www.bytearray.org/?p=4789
>>
>> --
>> Cosma
>>
>>
>> 2013/11/3 Maurice Amsellem <ma...@systar.com>
>>
>>> Thanks Justin
>>> ________________________________________
>>> De : Justin Mclean [justin@classsoftware.com]
>>> Envoyé : dimanche 3 novembre 2013 22:26
>>> À : dev@flex.apache.org
>>> Objet : Re: Need advice on patch validation
>>>
>>> Hi,
>>>
>>> > What should be the criteria for accepting an optimization proposition ?
>>> Does it improve significantly improve performance in real use cases for
>>> the majority of users would be at the tip of my list. Calling a method
>>> 10,000 times is sometimes not a real use case.
>>>
>>> > Can we also accept part of an optimization only ?
>>> Of course that's the whole point of submitting patches, commit then
>>> review etc etc
>>>
>>> Thanks,
>>> Justin
>>>
>>
>>
>
Re: Need advice on patch validation
Posted by Cosma Colanicchia <co...@gmail.com>.
... and also thanks to Justin for the extensive review work :)
2013/11/4 Cosma Colanicchia <co...@gmail.com>
> Thanks to Benoit, I can see the effort put into these patches.
>
> My general opinion is that we may choose to compromise code readability a
> little only when the optimization is really worth it, especially for
> public method signatures, because otherwise, applying the same logic
> extensively, we could end up with a SDK codebase very difficult to read,
> debug and improve. This readability-performance tradeoff may be pushed
> towards performance when required in method implementations (e.g. inlining
> simple method calls, reusing objects to reduce memory allocations, etc.)
>
> However, in this case, I agree in general with the use of indexOf instead
> of regexp, and to inlining these methods (if it can’t be done by the
> compiler, see next). The additional array argument could also be ok as we
> are talking about an Helper class, I see its public methods as an extension
> to the “helped” class, and not as a part of the “public API”. If this is
> not the general position, we could consider reaching the same result in
> some other way? For example, recycling static member array variables in the
> helper class to avoid reallocations (I think another performance patch
> related to object deserialization was adopting a similar pattern).
>
> Finally, as a side note, I read that Falcon is able
> to automatically inline method calls, optimizing the byte code at compile
> time (see [1] and [2] for example, at least it should be implemented in
> ASC2, don’t known if it also part of the donated Falcon codebase). If this
> is true, maybe we should give up this kind of manual optimization if we are
> going to switch to Falcon as the default compiler in the near future, or
> try to adhere to the requirements for this optimization to happen.
>
>
> (Just my two cents)
>
> [1] http://renaun.com/blog/2012/09/using-the-new-inline-metadata-in-asc2/
> [2] http://www.bytearray.org/?p=4789
>
> --
> Cosma
>
>
> 2013/11/3 Maurice Amsellem <ma...@systar.com>
>
>> Thanks Justin
>> ________________________________________
>> De : Justin Mclean [justin@classsoftware.com]
>> Envoyé : dimanche 3 novembre 2013 22:26
>> À : dev@flex.apache.org
>> Objet : Re: Need advice on patch validation
>>
>> Hi,
>>
>> > What should be the criteria for accepting an optimization proposition ?
>> Does it improve significantly improve performance in real use cases for
>> the majority of users would be at the tip of my list. Calling a method
>> 10,000 times is sometimes not a real use case.
>>
>> > Can we also accept part of an optimization only ?
>> Of course that's the whole point of submitting patches, commit then
>> review etc etc
>>
>> Thanks,
>> Justin
>>
>
>
RE: Need advice on patch validation
Posted by Maurice Amsellem <ma...@systar.com>.
Finally committed a modified version of the patch.
Thanks to all for your support and advice.
Thanks to Benoit for spotting the issue and providing the patch.
Regards,
Maurice
-----Message d'origine-----
De : Justin Mclean [mailto:justin@classsoftware.com]
Envoyé : lundi 4 novembre 2013 09:57
À : dev@flex.apache.org
Objet : Re: Need advice on patch validation
Hi,
> If this is not the general position, we could consider reaching the
> same result in some other way? For example, recycling static member
> array variables in the helper class to avoid reallocations
A good suggestion and than would cut down the memory allocation/CG cost, and given that it's an array with two numbers I don't think memory is the issue here. however in this case I'm not 100% what all the callers do with the returned array and in case I think its cached into an array which means we couldn't do that.
> Finally, as a side note, I read that Falcon is able to automatically
> inline method calls
>From memory you have to tell it do it and it's an all or noting affair which seems a bit restrictive to me, but certainly worth keeping in mind.
Thanks,
Justin
Re: Need advice on patch validation
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> If this is not the general position, we could consider reaching the same result in
> some other way? For example, recycling static member array variables in the
> helper class to avoid reallocations
A good suggestion and than would cut down the memory allocation/CG cost, and given that it's an array with two numbers I don't think memory is the issue here. however in this case I'm not 100% what all the callers do with the returned array and in case I think its cached into an array which means we couldn't do that.
> Finally, as a side note, I read that Falcon is able to automatically inline
> method calls
From memory you have to tell it do it and it's an all or noting affair which seems a bit restrictive to me, but certainly worth keeping in mind.
Thanks,
Justin
Re: Need advice on patch validation
Posted by Cosma Colanicchia <co...@gmail.com>.
Thanks to Benoit, I can see the effort put into these patches.
My general opinion is that we may choose to compromise code readability a
little only when the optimization is really worth it, especially for
public method signatures, because otherwise, applying the same logic
extensively, we could end up with a SDK codebase very difficult to read,
debug and improve. This readability-performance tradeoff may be pushed
towards performance when required in method implementations (e.g. inlining
simple method calls, reusing objects to reduce memory allocations, etc.)
However, in this case, I agree in general with the use of indexOf instead
of regexp, and to inlining these methods (if it can’t be done by the
compiler, see next). The additional array argument could also be ok as we
are talking about an Helper class, I see its public methods as an extension
to the “helped” class, and not as a part of the “public API”. If this is
not the general position, we could consider reaching the same result in
some other way? For example, recycling static member array variables in the
helper class to avoid reallocations (I think another performance patch
related to object deserialization was adopting a similar pattern).
Finally, as a side note, I read that Falcon is able to automatically inline
method calls, optimizing the byte code at compile time (see [1] and [2] for
example, at least it should be implemented in ASC2, don’t known if it also
part of the donated Falcon codebase). If this is true, maybe we should give
up this kind of manual optimization if we are going to switch to Falcon as
the default compiler in the near future, or try to adhere to the
requirements for this optimization to happen.
(Just my two cents)
[1] http://renaun.com/blog/2012/09/using-the-new-inline-metadata-in-asc2/
[2] http://www.bytearray.org/?p=4789
--
Cosma
2013/11/3 Maurice Amsellem <ma...@systar.com>
> Thanks Justin
> ________________________________________
> De : Justin Mclean [justin@classsoftware.com]
> Envoyé : dimanche 3 novembre 2013 22:26
> À : dev@flex.apache.org
> Objet : Re: Need advice on patch validation
>
> Hi,
>
> > What should be the criteria for accepting an optimization proposition ?
> Does it improve significantly improve performance in real use cases for
> the majority of users would be at the tip of my list. Calling a method
> 10,000 times is sometimes not a real use case.
>
> > Can we also accept part of an optimization only ?
> Of course that's the whole point of submitting patches, commit then review
> etc etc
>
> Thanks,
> Justin
>
RE:Need advice on patch validation
Posted by Maurice Amsellem <ma...@systar.com>.
Thanks Justin
________________________________________
De : Justin Mclean [justin@classsoftware.com]
Envoyé : dimanche 3 novembre 2013 22:26
À : dev@flex.apache.org
Objet : Re: Need advice on patch validation
Hi,
> What should be the criteria for accepting an optimization proposition ?
Does it improve significantly improve performance in real use cases for the majority of users would be at the tip of my list. Calling a method 10,000 times is sometimes not a real use case.
> Can we also accept part of an optimization only ?
Of course that's the whole point of submitting patches, commit then review etc etc
Thanks,
Justin
Re: Need advice on patch validation
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> What should be the criteria for accepting an optimization proposition ?
Does it improve significantly improve performance in real use cases for the majority of users would be at the tip of my list. Calling a method 10,000 times is sometimes not a real use case.
> Can we also accept part of an optimization only ?
Of course that's the whole point of submitting patches, commit then review etc etc
Thanks,
Justin