You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Lukas Eder <lu...@gmail.com> on 2013/03/06 11:48:09 UTC
Potential "code smell" in SlingPostServlet
Hello,
I've been stepping through the SlingPostServlet, having a look at its
code. I feel that there is a potential "code smell" in some of its
methods. As I'm unsure how the Sling project deals with such
situations, I felt that posting a mail here before creating an issue
might be better.
The code I'm talking about is contained in
registerPostResponseCreator(), where the cachedPostResponseCreators
attribute is initialised. There are two issues here:
1. The cachedPostResponseCreators array is re-initialised every time
this method is invoked. This leads to an unnecessary O(n^2)
initialisation complexity on componentContext.locateService() method.
I guess that this cache's whole purpose is to avoid expensive
locateService() calls, so this should probably be fixed.
2. A first loop initialises this cachedPostResponseCreators array. In
a second step, "null" entries are "removed" by copying the array. This
behaviour is hard to read from the existing code, which is a bit
verbose. In order to avoid the potential for bugs, I'd like to suggest
using the (untested) behaviour from the attached patch. Note that
other methods, such as registerPostProcessor(),
registerNodeNameGenerator() suffers from similar "verbosity problems".
Cheers
Lukas
Re: Potential "code smell" in SlingPostServlet
Posted by Felix Meschberger <fm...@adobe.com>.
Am 06.03.2013 um 14:37 schrieb Lukas Eder:
> Hello Antonio,
>
> 2013/3/6 Antonio Sanso <as...@adobe.com>:
>> Hi Lukas
>>
>> On Mar 6, 2013, at 11:48 AM, Lukas Eder wrote:
>>
>>> Hello,
>>>
>>> I've been stepping through the SlingPostServlet, having a look at its
>>> code. I feel that there is a potential "code smell" in some of its
>>> methods. As I'm unsure how the Sling project deals with such
>>> situations, I felt that posting a mail here before creating an issue
>>> might be better.
>>>
>>> The code I'm talking about is contained in
>>> registerPostResponseCreator(), where the cachedPostResponseCreators
>>> attribute is initialised. There are two issues here:
>>>
>>> 1. The cachedPostResponseCreators array is re-initialised every time
>>> this method is invoked.
>>
>> if I read correctly the code this method is called only at bundle activation and binding
>
> Yes, that's true. For instance, when the post-servlet-extensions
> sample is loaded through the OSGi console, this code is executed. It's
> probably only a minor annoyance for most users that do not have an
> overly excessive amount of PostResponseCreators :-)
Its not an issue at all. We don't expect (a) a large, aka >100 or more, number of PostResponseCreator services and (b) unregistration is done through unbindPostResponseCreator. Everything should work fine and now problem should be expected to the system load. This is just regular maintenance.
As for ComponentContext.locateService: The result of this call is cached in the ComponentContext. Even if it would not be, the call itself is not too overly expensive.
>
>> Regards
>>
>> Antonio
>>
>>> This leads to an unnecessary O(n^2)
>>> initialisation complexity on componentContext.locateService() method.
There is no initialization involved here: this just accesses a potential cache in the ComponentContext for the named service or -- if missing from the cache -- calls back to the OSGi framework for the service.
Regards
Felix
>>> I guess that this cache's whole purpose is to avoid expensive
>>> locateService() calls, so this should probably be fixed.
>>>
>>> 2. A first loop initialises this cachedPostResponseCreators array. In
>>> a second step, "null" entries are "removed" by copying the array. This
>>> behaviour is hard to read from the existing code, which is a bit
>>> verbose. In order to avoid the potential for bugs, I'd like to suggest
>>> using the (untested) behaviour from the attached patch. Note that
>>> other methods, such as registerPostProcessor(),
>>> registerNodeNameGenerator() suffers from similar "verbosity problems".
>>>
>>> Cheers
>>> Lukas
>>
--
Felix Meschberger | Principal Scientist | Adobe
Re: Potential "code smell" in SlingPostServlet
Posted by Lukas Eder <lu...@gmail.com>.
Hello Antonio,
2013/3/6 Antonio Sanso <as...@adobe.com>:
> Hi Lukas
>
> On Mar 6, 2013, at 11:48 AM, Lukas Eder wrote:
>
>> Hello,
>>
>> I've been stepping through the SlingPostServlet, having a look at its
>> code. I feel that there is a potential "code smell" in some of its
>> methods. As I'm unsure how the Sling project deals with such
>> situations, I felt that posting a mail here before creating an issue
>> might be better.
>>
>> The code I'm talking about is contained in
>> registerPostResponseCreator(), where the cachedPostResponseCreators
>> attribute is initialised. There are two issues here:
>>
>> 1. The cachedPostResponseCreators array is re-initialised every time
>> this method is invoked.
>
> if I read correctly the code this method is called only at bundle activation and binding
Yes, that's true. For instance, when the post-servlet-extensions
sample is loaded through the OSGi console, this code is executed. It's
probably only a minor annoyance for most users that do not have an
overly excessive amount of PostResponseCreators :-)
> Regards
>
> Antonio
>
>> This leads to an unnecessary O(n^2)
>> initialisation complexity on componentContext.locateService() method.
>> I guess that this cache's whole purpose is to avoid expensive
>> locateService() calls, so this should probably be fixed.
>>
>> 2. A first loop initialises this cachedPostResponseCreators array. In
>> a second step, "null" entries are "removed" by copying the array. This
>> behaviour is hard to read from the existing code, which is a bit
>> verbose. In order to avoid the potential for bugs, I'd like to suggest
>> using the (untested) behaviour from the attached patch. Note that
>> other methods, such as registerPostProcessor(),
>> registerNodeNameGenerator() suffers from similar "verbosity problems".
>>
>> Cheers
>> Lukas
>
Re: Potential "code smell" in SlingPostServlet
Posted by Carsten Ziegeler <cz...@apache.org>.
2013/3/6 Antonio Sanso <as...@adobe.com>:
> Hi Lukas
>
> On Mar 6, 2013, at 11:48 AM, Lukas Eder wrote:
>
>> Hello,
>>
>> I've been stepping through the SlingPostServlet, having a look at its
>> code. I feel that there is a potential "code smell" in some of its
>> methods. As I'm unsure how the Sling project deals with such
>> situations, I felt that posting a mail here before creating an issue
>> might be better.
>>
>> The code I'm talking about is contained in
>> registerPostResponseCreator(), where the cachedPostResponseCreators
>> attribute is initialised. There are two issues here:
>>
>> 1. The cachedPostResponseCreators array is re-initialised every time
>> this method is invoked.
>
> if I read correctly the code this method is called only at bundle activation and binding
Yes, that's right - however briefly looking at the code, it smells as
this cache is not really updated for an unbind.
Regards
Carsten
>
> Regards
>
> Antonio
>
>> This leads to an unnecessary O(n^2)
>> initialisation complexity on componentContext.locateService() method.
>> I guess that this cache's whole purpose is to avoid expensive
>> locateService() calls, so this should probably be fixed.
>>
>> 2. A first loop initialises this cachedPostResponseCreators array. In
>> a second step, "null" entries are "removed" by copying the array. This
>> behaviour is hard to read from the existing code, which is a bit
>> verbose. In order to avoid the potential for bugs, I'd like to suggest
>> using the (untested) behaviour from the attached patch. Note that
>> other methods, such as registerPostProcessor(),
>> registerNodeNameGenerator() suffers from similar "verbosity problems".
>>
>> Cheers
>> Lukas
>
--
Carsten Ziegeler
cziegeler@apache.org
Re: Potential "code smell" in SlingPostServlet
Posted by Antonio Sanso <as...@adobe.com>.
Hi Lukas
On Mar 6, 2013, at 11:48 AM, Lukas Eder wrote:
> Hello,
>
> I've been stepping through the SlingPostServlet, having a look at its
> code. I feel that there is a potential "code smell" in some of its
> methods. As I'm unsure how the Sling project deals with such
> situations, I felt that posting a mail here before creating an issue
> might be better.
>
> The code I'm talking about is contained in
> registerPostResponseCreator(), where the cachedPostResponseCreators
> attribute is initialised. There are two issues here:
>
> 1. The cachedPostResponseCreators array is re-initialised every time
> this method is invoked.
if I read correctly the code this method is called only at bundle activation and binding
Regards
Antonio
> This leads to an unnecessary O(n^2)
> initialisation complexity on componentContext.locateService() method.
> I guess that this cache's whole purpose is to avoid expensive
> locateService() calls, so this should probably be fixed.
>
> 2. A first loop initialises this cachedPostResponseCreators array. In
> a second step, "null" entries are "removed" by copying the array. This
> behaviour is hard to read from the existing code, which is a bit
> verbose. In order to avoid the potential for bugs, I'd like to suggest
> using the (untested) behaviour from the attached patch. Note that
> other methods, such as registerPostProcessor(),
> registerNodeNameGenerator() suffers from similar "verbosity problems".
>
> Cheers
> Lukas
Re: Potential "code smell" in SlingPostServlet
Posted by Lukas Eder <lu...@gmail.com>.
Hello Antonio,
2013/3/6 Antonio Sanso <as...@adobe.com>:
> Hi Lukas
>
> On Mar 6, 2013, at 11:48 AM, Lukas Eder wrote:
>
>> Hello,
>>
>> I've been stepping through the SlingPostServlet, having a look at its
>> code. I feel that there is a potential "code smell" in some of its
>> methods. As I'm unsure how the Sling project deals with such
>> situations, I felt that posting a mail here before creating an issue
>> might be better.
>>
>> The code I'm talking about is contained in
>> registerPostResponseCreator(), where the cachedPostResponseCreators
>> attribute is initialised. There are two issues here:
>>
>> 1. The cachedPostResponseCreators array is re-initialised every time
>> this method is invoked. This leads to an unnecessary O(n^2)
>> initialisation complexity on componentContext.locateService() method.
>> I guess that this cache's whole purpose is to avoid expensive
>> locateService() calls, so this should probably be fixed.
>>
>> 2. A first loop initialises this cachedPostResponseCreators array. In
>> a second step, "null" entries are "removed" by copying the array. This
>> behaviour is hard to read from the existing code, which is a bit
>> verbose. In order to avoid the potential for bugs, I'd like to suggest
>> using the (untested) behaviour from the attached patch.
>
> I am afraid the attachment has been blocked.
> You might think to open a JIRA issue.
Sorry about that. Here's the JIRA issue for this problem:
https://issues.apache.org/jira/browse/SLING-2774
> Regards
>
> Antonio
>
>
>> Note that
>> other methods, such as registerPostProcessor(),
>> registerNodeNameGenerator() suffers from similar "verbosity problems".
>>
>> Cheers
>> Lukas
>
Re: Potential "code smell" in SlingPostServlet
Posted by Antonio Sanso <as...@adobe.com>.
Hi Lukas
On Mar 6, 2013, at 11:48 AM, Lukas Eder wrote:
> Hello,
>
> I've been stepping through the SlingPostServlet, having a look at its
> code. I feel that there is a potential "code smell" in some of its
> methods. As I'm unsure how the Sling project deals with such
> situations, I felt that posting a mail here before creating an issue
> might be better.
>
> The code I'm talking about is contained in
> registerPostResponseCreator(), where the cachedPostResponseCreators
> attribute is initialised. There are two issues here:
>
> 1. The cachedPostResponseCreators array is re-initialised every time
> this method is invoked. This leads to an unnecessary O(n^2)
> initialisation complexity on componentContext.locateService() method.
> I guess that this cache's whole purpose is to avoid expensive
> locateService() calls, so this should probably be fixed.
>
> 2. A first loop initialises this cachedPostResponseCreators array. In
> a second step, "null" entries are "removed" by copying the array. This
> behaviour is hard to read from the existing code, which is a bit
> verbose. In order to avoid the potential for bugs, I'd like to suggest
> using the (untested) behaviour from the attached patch.
I am afraid the attachment has been blocked.
You might think to open a JIRA issue.
Regards
Antonio
> Note that
> other methods, such as registerPostProcessor(),
> registerNodeNameGenerator() suffers from similar "verbosity problems".
>
> Cheers
> Lukas