click-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bob Schellink <sab...@gmail.com>
Subject Re: Issues or no issue...
Date Mon, 14 Jun 2010 23:34:50 GMT
Hi Jahid,

On 15/06/2010 02:33, Md. Jahid Shohel wrote:

> 
> 2) The same way DeployUtils class is package local, and I made a true copy.


I assume you needed this for #3? See my next comment.


> 3) The org.apache.Control interface was made to be both "a generic
> control" and "a deploy aware control". Which I felt more of mixing of
> concern. Shouldn't these be two separate interfaces. Like -
> 
> public interface DeployAwareControl{
>      public void onDeploy(ServletContext context);
> }
> 
> And for backward compatibility we can make Control interface extending
> DeployAwareControl.


Click provides two ways of deploying resources. Control#onDeploy and the Servlet3.0 approach
of
packaging resources under /META-INF/resources. I think Control#onDeploy should be deprecated
as it
has some major drawbacks:

 - Control has to be instantiated during startup in order to invoke onDeploy. At startup time
there
is no request, and thus no Context object. The net effect of this is that the Control cannot
rely on
Context in it's constructor. Accessing Context in the Control constructor is useful because
one can
pull data from the Session to retrieve state.
 - changes to the resources have to be updated in two places, once for getHeadElements and
again for
onDeploy


> 4) The "isTemplate" method is having a bug. In the method we are not
> checking if the file is really a file or folder, Because on my Linux
> (Ubuntu) I was able to create a folder with .htm  or .jsp extension. I
> know this might not happen so often, but still I think it might create
> problem.


isTemplate is used at runtime and would create overhead if we were to touch the filesystem.
I don't
see a use case for having .htm and .jsp named folders inside the webapp. That said what is
the
behavior of having such a folder defined?


> 6) When the application is loading, we load all page classes, and their
> bindable page fields. Could be a memory issue when benchmarking Click
> against other frameworks. We can load bindable page fields on demand,
> and cache them. That way we will not end up loading all page fields even
> if its not needed. For an application with 100 pages this could be a
> performance issue. As an example: say on http://click.apache.org there
> are lots of pages which is not browsed by user (example license content
> page, how to release, and pages like that), those pages will still be
> loaded. I know an application running for long time might be loading all
> pages eventually. But still, we can load them on demand, and we can
> cache loaded classes. And we can also keep the house keeping thread to
> eventually clear up rarely loaded pages.


I'd like to see lazy loading as well so feel free to open a JIRA for this. Not only does it
affect
startup but it will allow Click to run in production mode under GAE[1]. GAE does not support
the
method servletContext.getResourcePaths which XmlConfigService uses internally. Lazy loading
does
present its own problems such as synchronizing the data structures used to store page classes
and
templates.


> 7) On "getBindablePageFields" method of XmlConfigService, this piece of
> code -
>    
> Field[] publicFields = pageClass.getFields();
>     for (Field field : publicFields) {
>         fieldMap.put(field.getName(), field);
> }
> 
> Though @Bindable fields are still processed, but here we again we are
> processing all public fields (even though they have @Bindable and
> already processed). Also there are some repetition of code (on if-else)
> block. Possible to improve. Will help to get a better picture when
> benchmarking.


What optimization do you have in mind?


Kind regards

Bob

[1]: https://issues.apache.org/jira/browse/CLK-639

Mime
View raw message