myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Struberg <strub...@yahoo.de>
Subject Re: StateSaving review
Date Thu, 15 Nov 2012 23:39:14 GMT


Grant, there is no change. Compare the versions. If we take back the hashCode then it's really
only moving inner classes to own java files and remove duplicate code. Those tons of classes
haven't been written by me, they have ALL been in the code already - just hidden as 1000++
LOC of inner classes in a single file. No wonder that we didn't ever review them properly.

LieGrue,
strub


>________________________________
> From: Grant Smith <grant@marathonpm.com>
>To: MyFaces Development <dev@myfaces.apache.org> 
>Sent: Friday, November 16, 2012 12:30 AM
>Subject: Re: StateSaving review
> 
>
>I have to say, as an observer of this conversation it makes sense to work in 2.1.x-client-window,
and let Leo proceed with the release process without having to deal with vetting changes which
could possible effect stability. We have far more to lose by working in the 2.1.x branch.
>
>-Grant
>
>
>
>
>On Thu, Nov 15, 2012 at 3:11 PM, Leonardo Uribe <lu4242@gmail.com> wrote:
>
>Hi
>>
>>2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>
>>> Leo, it would be great if you reflect a bit.
>>>
>>>> I have checked the code. I know what SerializedViewKey does.
>>>
>>> good! no other arguments? Well, then all is fine it seems.
>>>
>>>
>>>> it is YOU the one who are reverting code done by other people
>>>
>>> The only code I ever reverted in MyFaces was your revert of my code change. That's
it.
>>
>>But you reverted the changes I did months ago. Didn't you? oh yes, you
>>"refactor" it and
>>get rid what you don't like.
>>
>>
>>> But actually I'm not the only one who got code reverted by you, and those people
are fed up as well and partly left the community already because of that.
>>>
>>
>>That's unfair. I don't think that's true. I'm just exercising my right
>>to participate, that's it. People can disagree from time to time.
>>That's part of the life, if you can't deal with it, it's your problem.
>>But I'm always willing to look for alternatives and resolve
>>differences, like I'm doing right now, and that's what makes the
>>difference.
>>
>>
>>>
>>>> I'm sorry to be so direct, but as a member of the community, I
>>>> have the right to participate and I'm willing to exercise that right.
>>>
>>> That's perfectly fine as well. So please start your excercise with listening
to other people sometimes.
>>> * I listed 5++ flaws in the design.
>>> * I removed a few classes which contained 99% copy & paste.
>>> * I removed a few classes which are not used anymore since over a year
>>> * I removed an old resources folder which was just lying around for no reason.
>>> * I moved the remaining inner classes to toplevel and an own package to make
it easier to maintain and overlook the whole complexity.
>>> * I added the viewId to _one_ of the dozen key implementations. Actually we don't
need it. We also don't need the viewId hashCode which was there before. So I don't care, let's
move this back to your hashCode solution and fix it properly in the next release.
>>> * I cleaned up your generics because it always only used the String type anyway.
And this cannot change in the future because this fact comes directly from the JSF spec.
>>> Otherwise there has been no fundamental change to your code. The mess you see
is all yours so to say (sorry for the sarcasm).
>>>
>>
>>That's your opinion, not mine. But I can see some advance. At last, there is
>>one list of the changes proposed. Since the plan is do a release next
>>week, could
>>we revert the changes, and put them on hold while the necessary steps are done?
>>Otherwise a release will not be possible for a long time (In fact, you
>>have kidnapped
>>the release). In that way, we can dedicate enough time and resources to get
>>it out once for all. Do you agree with that? Could you let me complete
>>the planned
>>schedule?
>>
>>Anyway, what's the deal to put the changes in 2.1.x branch? why you are so
>>desperate for that? You have not given to me any reason why you can't work with
>>2.1.x-client-window branch, which is best for what you are trying to
>>do, or why you
>>can wait just 2 weeks to commit the changes.
>>
>>Come on Mark, is not a big deal, right?
>>
>>
>>>
>>>> The concrete case is spring webflow does not support PSS algorithm, so
>>>> we need to switch back and enable compatibility mode with JSF 1.2.
>>>
>>> Spring WebFlow uses it's own StateManager which natively extends javax.faces.StateManager.
There is no MyFaces code involved anymore since ages!
>>> http://static.springsource.org/spring-webflow/docs/2.0.x/javadoc-api/org/springframework/faces/webflow/FlowViewStateManager.html
>>>
>>> I also googled quite some time and fine nothing pointing to any problems with
the MyFaces StateManager. Not even on stackoverflow.
>>>
>>
>>Look this:
>>
>>https://jira.springsource.org/browse/SWF-1365
>>
>>Because that problem, I decided to preserve JspStateManagerImpl long time ago,
>>specifically when MYFACES-3117 was solved, because the change over
>>ResponseStateManager imposed a change over all state management in MyFaces.
>>Is there a problem? don't care if exists or not, what's important is provide
>>a way to go back to the old behavior. Be conservative, keep code stable.
>>
>>
>>regards,
>>
>>Leonardo Uribe
>>
>>>
>>>
>>> LieGrue,
>>> strub
>>>
>>>
>>>
>>> ----- Original Message -----
>>>> From: Leonardo Uribe <lu4242@gmail.com>
>>>> To: MyFaces Development <dev@myfaces.apache.org>; Mark Struberg <struberg@yahoo.de>
>>>> Cc:
>>>> Sent: Thursday, November 15, 2012 11:06 PM
>>>> Subject: Re: StateSaving review
>>>>
>>>> Hi
>>>>
>>>> 2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>>>
>>>>>>  You did more than that. You removed the following classes:
>>>>>>
>>>>>>  IntIntSerializedViewKey
>>>>>>  IntByteArraySerializedViewKey
>>>>>>  ReferenceSerializedViewKey
>>>>>
>>>>>  Those have been almost 1:1 clones of other classes. Go check
>>>> SerializedViewKey, it contains all the merged functionality. Without any
>>>> performance overhead.
>>>>>  Look at the code first and then you can complain. You revert code done
by
>>>> other people without even looking at the changes it seems.
>>>>>
>>>>>
>>>>
>>>> I have checked the code. I know what SerializedViewKey does.
>>>> I'm pretty aware of what it does and its effects. I have to say it,
>>>> it is YOU the one who are reverting code done by other people
>>>> without even looking at the changes, just because you have
>>>> the perception that the changes are wrong, but nothing more.
>>>> Without any proof or any deep consideration.
>>>>
>>>> The changes I have done over the time has follow the same
>>>> protocol, create an issue, provide a patch, wait for feedback of
>>>> the community, if no objections after a prudent time commit them
>>>> and close the issue. In this moment the changes done by me has
>>>> the tacit aval of the community. Your changes do not have that,
>>>> because they have objections done 72 hours after they were
>>>> proposed. This is a meritocracy, and the rules of the community
>>>> in this case are on my side.
>>>>
>>>> I'm sorry to be so direct, but as a member of the community, I
>>>> have the right to participate and I'm willing to exercise that right.
>>>> I will not look to other side and let some untested code go into
>>>> myfaces core 2.1.x. I appreciate your efforts to make this part
>>>> better, but I encourage you to respect and play with the rules
>>>> of the community.
>>>>
>>>> Look the dates of the improvements:
>>>>
>>>> MYFACES-3563
>>>> Created: 10/Jun/12 11:33
>>>> Resolved: 25/Jun/12 09:05
>>>>
>>>> MYFACES-3117
>>>> Created: 26/Apr/11 19:08
>>>> Resolved: 14/May/11 02:27
>>>>
>>>> That code has been there for years!!!!!! , the latest changes were done
>>>> 5 months ago !!!. Nobody complained at that time. Now you came here,
>>>> complaining about the code and you want to fix it without any interest
>>>> of what others think about that. That's not cool.
>>>>
>>>>>
>>>>>>  and on top of that you removed JspStateManagerImpl, which is a
class
>>>>>>  that is there for compatibility with old state managers that relies
on
>>>>>>  MyFaces 1.2.x.
>>>>>
>>>>>  This is not JSF-1.x since a long time. APIs in the StateManager got
changed
>>>> for AJAX support, etc.
>>>>>
>>>>>  But I'm all fine with going back to the JspStateManagerImpl again as
I
>>>> already said.
>>>>>
>>>>>  Btw, may I quote your answer to my question if I can delete it?
>>>>>  LU> "Remove that code is a valid option."
>>>>>
>>>>
>>>> The concrete case is spring webflow does not support PSS algorithm, so
>>>> we need to switch back and enable compatibility mode with JSF 1.2.
>>>> JspStateManagerImpl is there because the state manager in that library
>>>> relies on the old behavior. I think there are other cases too. But I hope
to
>>>> remove it in the future (JSF 2.2).
>>>>
>>>> regards,
>>>>
>>>> Leonardo Uribe
>>>>
>>>>>  LieGrue,
>>>>>  strub
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>  ----- Original Message -----
>>>>>>  From: Leonardo Uribe <lu4242@gmail.com>
>>>>>>  To: MyFaces Development <dev@myfaces.apache.org>; Mark Struberg
>>>> <struberg@yahoo.de>
>>>>>>  Cc:
>>>>>>  Sent: Thursday, November 15, 2012 10:20 PM
>>>>>>  Subject: Re: StateSaving review
>>>>>>
>>>>>>  Hi
>>>>>>
>>>>>>  2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>>>>>   Leo, the stuff which currently got changed is nothing more
than
>>>> just your
>>>>>>  code. I only refactored it into an own package and moved the inner
>>>> classes to
>>>>>>  toplevel to show up the complexity of the solution. There was no
code
>>>> change
>>>>>>  _yet_ other than the viewId. We can change back the viewId to the
hash
>>>> if you
>>>>>>  feel better. It makes no difference anyway.
>>>>>>>
>>>>>>
>>>>>>  You did more than that. You removed the following classes:
>>>>>>
>>>>>>  IntIntSerializedViewKey
>>>>>>  IntByteArraySerializedViewKey
>>>>>>  ReferenceSerializedViewKey
>>>>>>
>>>>>>  also you created this one (it was an abstract class before)
>>>>>>
>>>>>>  SerializedViewKey
>>>>>>
>>>>>>  and on top of that you removed JspStateManagerImpl, which is a
class
>>>>>>  that is there for compatibility with old state managers that relies
on
>>>>>>  MyFaces 1.2.x. Obviously you have not considered those cases, but
I
>>>> have
>>>>>>  done that long time ago.
>>>>>>
>>>>>>  Without ask, without propose a patch, without let the people who
care
>>>> about
>>>>>>  to participate. Just do it and let others do the clean up.
>>>>>>
>>>>>>  Mark, I have came here and proposed to you many possible alternatives
>>>>>>  and you're letting me without choices. I beg you to move your
>>>> changes to
>>>>>>  2.1.x-client-window branch, and when we have certain about which
>>>> changes
>>>>>>  needs to be done, backport them to 2.1.x. In just one step, without
>>>> this
>>>>>>  pain and stress you are causing me. Please. The reason why it was
>>>> created
>>>>>>  2.1.x-client-window was precisely to try changes for client window
/
>>>> state
>>>>>>  manager and flash scope.
>>>>>>
>>>>>>  In this moment I can't do any release. You have tied my hands.
>>>> I'm
>>>>>>  trying to be
>>>>>>  kind, please.
>>>>>>
>>>>>>>   In the long run this stuff needs _heavy_ cleanup.
>>>>>>>   Again: Extracting out an abstraction over Server vs Client
>>>> stateSaving is
>>>>>>  perfectly fine, but the code is now way more complicated than it
ever
>>>> has been.
>>>>>>  Premature abstraction is way worse than premature optimisation.
>>>>>>>
>>>>>>
>>>>>>  It is a work in progress, I already told you many times. I know
the
>>>> abstraction
>>>>>>  is not the best, but it works, and has helped a lot to me to understand
>>>> how
>>>>>>  this algorithm should work. It is like a worm that became chrysalis
and
>>>> in the
>>>>>>  future will become a butterfly. "... the morphing step is not nice
>>>>>>  ..."
>>>>>>
>>>>>>  regards,
>>>>>>
>>>>>>  Leonardo
>>>>>>
>>>>>>>   LieGrue,
>>>>>>>   strub
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>   ----- Original Message -----
>>>>>>>>   From: Leonardo Uribe <lu4242@gmail.com>
>>>>>>>>   To: MyFaces Development <dev@myfaces.apache.org>;
Mark
>>>> Struberg
>>>>>>  <struberg@yahoo.de>
>>>>>>>>   Cc:
>>>>>>>>   Sent: Thursday, November 15, 2012 9:30 PM
>>>>>>>>   Subject: Re: StateSaving review
>>>>>>>>
>>>>>>>>   Hi
>>>>>>>>
>>>>>>>>   2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>>>>>>>    Leo, I'm ok with a revert. But not to your version
>>>> but to the
>>>>>>  original
>>>>>>>>   JspStateManagerImpl!
>>>>>>>>>
>>>>>>>>>    Really, this currently feels so overcomplicated
without
>>>> giving
>>>>>>  much
>>>>>>>>   benefit. It would have been perfectly enough to remove
the
>>>> viewId
>>>>>>  String and
>>>>>>>>   replace it with a XORShift random value if you don't trust
>>>> the
>>>>>>  sequencer.
>>>>>>>>>
>>>>>>>>>    I know one goal was to abstract out the state between
>>>> client and
>>>>>>  server.
>>>>>>>>   But that doesn't mean that we end up with Object, Object
>>>> and do
>>>>>>  hardcoded
>>>>>>>>   upcasts as done right now in the code. This is no improvement
>>>> over the
>>>>>>  original
>>>>>>>>   code. The windowId change would have been 30 LOC btw.
>>>>>>>>>
>>>>>>>>
>>>>>>>>   It is fine if you want to change the implementation to
make
>>>> something
>>>>>>>>   better. What's not fine, is that you want to work in 2.1.x
>>>> branch
>>>>>>>>   directly, that is right now in maintenance stage. So,
you are
>>>> doing
>>>>>>>>   all kind of refactorings, and that makes me crazy. The
last
>>>> release
>>>>>>>>   was done on 23/Sep/12 and I'm doing 1 release each 2
>>>> months. Your
>>>>>>>>   changes will take longer and I need time to review them,
>>>> because I
>>>>>>>>   will not do a release over changes that I have not seen.
>>>>>>>>
>>>>>>>>   My idea is start a release process next week. Why don't
>>>> you take
>>>>>>  your
>>>>>>>>   time?, instead you try to do changes without any clear
>>>> direction.
>>>>>>>>
>>>>>>>>   In this moments, we are starting the necessary steps to
work
>>>> on 2.2.x.
>>>>>>>>   You should be working in 2.1.x-client-window branch !!!!!
>>>>>>>>
>>>>>>>>   regards,
>>>>>>>>
>>>>>>>>   Leonardo Uribe
>>>>>>>>
>>>>>>>>>    LieGrue,
>>>>>>>>>    strub
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>    ----- Original Message -----
>>>>>>>>>>    From: Leonardo Uribe <lu4242@gmail.com>
>>>>>>>>>>    To: MyFaces Development
>>>> <dev@myfaces.apache.org>; Mark
>>>>>>  Struberg
>>>>>>>>   <struberg@yahoo.de>
>>>>>>>>>>    Cc:
>>>>>>>>>>    Sent: Thursday, November 15, 2012 8:44 PM
>>>>>>>>>>    Subject: Re: StateSaving review
>>>>>>>>>>
>>>>>>>>>>    Hi
>>>>>>>>>>
>>>>>>>>>>    2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>>>>>>>>>     Btw, I still do not see where the trick
for
>>>> storing a
>>>>>>  separate
>>>>>>>>   state list
>>>>>>>>>>    for each browser tab is hidden.
>>>>>>>>>>>     That was the goal of all the refactoring
>>>> initially. Where
>>>>>>  is that
>>>>>>>>   done?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>    Mark, months ago we created 2.1.x-client-window
>>>> branch:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>> http://svn.apache.org/repos/asf/myfaces/core/branches/2.1.x-client-window/
>>>>>>>>>>
>>>>>>>>>>    This branch is the same as 2.1.x, but only with
the
>>>> changes
>>>>>>  proposed
>>>>>>>>>>    long time ago for JSF 2.2 client window.
>>>>>>>>>>
>>>>>>>>>>    It has what you need to have a state list for
each
>>>> browser
>>>>>>  tab.
>>>>>>>>>>    The only thing you need to provide is a ClientWindow
>>>>>>  implementation
>>>>>>>>>>    and that's it. The state saving code already
has
>>>> included
>>>>>>  the
>>>>>>>>   client-window
>>>>>>>>>>    concept. I also fixed Flash scope to use client
>>>> window.
>>>>>>>>>>    It is also available in a maver repo:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>> https://repository.apache.org/content/repositories/snapshots/org/apache/myfaces/core/myfaces-bundle/
>>>>>>>>>>
>>>>>>>>>>    Let's make a deal. Move your changes to that
>>>> branch, let
>>>>>>  me apply
>>>>>>>>   my fix
>>>>>>>>>>    in trunk and do a release. When your changes
are
>>>> ready, we can
>>>>>>  discuss
>>>>>>>>>>    them and backport from 2.1.x-client-window to
trunk.
>>>> Does that
>>>>>>  sound
>>>>>>>>>>    good for you?
>>>>>>>>>>
>>>>>>>>>>    regards,
>>>>>>>>>>
>>>>>>>>>>    Leonardo Uribe
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>     LieGrue,
>>>>>>>>>>>     strub
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>     ----- Original Message -----
>>>>>>>>>>>>     From: Mark Struberg
>>>> <struberg@yahoo.de>
>>>>>>>>>>>>     To: MyFaces Development
>>>>>>  <dev@myfaces.apache.org>
>>>>>>>>>>>>     Cc:
>>>>>>>>>>>>     Sent: Thursday, November 15, 2012 6:32
PM
>>>>>>>>>>>>     Subject: Re: StateSaving review
>>>>>>>>>>>>
>>>>>>>>>>>>     I did further reviews:
>>>>>>>>>>>>
>>>>>>>>>>>>     Currently the CounterKeyFactory needs
some
>>>> random to
>>>>>>  be unique
>>>>>>>>>>    (according to
>>>>>>>>>>>>     Leo) and the RandomKeyFactory needs
a
>>>> counter to be
>>>>>>  unique.
>>>>>>>>>>>>
>>>>>>>>>>>>     Does that ring a bell? That stuff is
>>>> completely
>>>>>>  useless!
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     Foooooolks wake up, let's provide ONE
>>>> factory
>>>>>>  which is
>>>>>>>>   waterproof.
>>>>>>>>>>    That
>>>>>>>>>>>>     would be much better than having 15++
>>>> classes full of
>>>>>>  half
>>>>>>>>   baken/broken
>>>>>>>>>>>>     algorithms.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     Proposal: Get rid of all that KeyFactory
>>>> stuff again
>>>>>>  and have
>>>>>>>>   one GOOD
>>>>>>>>>>>>     algorithm: Counter + XORShift Random.
>>>>>>>>>>>>
>>>>>>>>>>>>     The KeyFactory stuff got only introduced
in
>>>> 2.1.9 and
>>>>>>  is
>>>>>>>>   crashing in
>>>>>>>>>>    production.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     LieGrue,
>>>>>>>>>>>>     strub
>>>>>>>>>>>>
>>>>>>>>>>>>     PS: don't take it personal, but from
>>>> looking at
>>>>>>  the code I
>>>>>>>>   see
>>>>>>>>>>    clearly what
>>>>>>>>>>>>     happens if someone works on a stuff
alone
>>>> without
>>>>>>  reviews.
>>>>>>>>   This always
>>>>>>>>>>    leads to
>>>>>>>>>>>>     deficits, regardless how good one is.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     ----- Original Message -----
>>>>>>>>>>>>>      From: Leonardo Uribe
>>>> <lu4242@gmail.com>
>>>>>>>>>>>>>      To: MyFaces Development
>>>>>>  <dev@myfaces.apache.org>;
>>>>>>>>   Mark
>>>>>>>>>>    Struberg
>>>>>>>>>>>>     <struberg@yahoo.de>
>>>>>>>>>>>>>      Cc:
>>>>>>>>>>>>>      Sent: Thursday, November 15,
2012 6:20
>>>> PM
>>>>>>>>>>>>>      Subject: Re: StateSaving review
>>>>>>>>>>>>>
>>>>>>>>>>>>>      Hi
>>>>>>>>>>>>>
>>>>>>>>>>>>>      2012/11/15 Mark Struberg
>>>>>>  <struberg@yahoo.de>:
>>>>>>>>>>>>>>       I'm currently reviewing
all
>>>> this area.
>>>>>>  It seems
>>>>>>>>   that we
>>>>>>>>>>    have quite
>>>>>>>>>>>>     some
>>>>>>>>>>>>>      stuff to improve.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>       a.) just a gut feeling yet,
but my
>>>> tummy
>>>>>>  tells me
>>>>>>>>   that we
>>>>>>>>>>    have to
>>>>>>>>>>>>     review
>>>>>>>>>>>>>      our key generator/storage strategies.
>>>> Too
>>>>>>  complicated or
>>>>>>>>   too badly
>>>>>>>>>>>>     documented.
>>>>>>>>>>>>>      At least they are not self describing.
>>>>>>>>>>>>>
>>>>>>>>>>>>>      It's a complex algorithm. No
>>>> documentation
>>>>>>  so far,
>>>>>>>>   because it
>>>>>>>>>>    is still
>>>>>>>>>>>>>      work in progress.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>       b.) candidate 1:
>>>> CounterKeyFactory. If we
>>>>>>  like to
>>>>>>>>   prevent
>>>>>>>>>>    reboot
>>>>>>>>>>>>     clashes
>>>>>>>>>>>>>      then we might add another int
which
>>>> contains a
>>>>>>  random
>>>>>>>>   value. Think
>>>>>>>>>>    about
>>>>>>>>>>>>     having
>>>>>>>>>>>>>      a Server with a single page right
now.
>>>> Click on
>>>>>>  it a few
>>>>>>>>   times,
>>>>>>>>>>    then
>>>>>>>>>>>>     restart
>>>>>>>>>>>>>      myfaces and issue a few requests
to the
>>>> same
>>>>>>  page and go
>>>>>>>>   back in
>>>>>>>>>>    your
>>>>>>>>>>>>     browser
>>>>>>>>>>>>>      history. Proposal: instead of
the
>>>> viewId we
>>>>>>  should add a
>>>>>>>>   random
>>>>>>>>>>    number.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>      Since the counter is also stored
into
>>>> session,
>>>>>>  the last
>>>>>>>>   counter
>>>>>>>>>>    values
>>>>>>>>>>>>>      is not lost.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>       c.) a general one. We might
>>>> introduce an
>>>>>>  own Random
>>>>>>>>   which
>>>>>>>>>>    either uses
>>>>>>>>>>>>>      java.util.concurrent.ThreadLocalRandom
>>>> for java
>>>>>>  7++ or
>>>>>>>>   the old
>>>>>>>>>>    Random impl.
>>>>>>>>>>>>>>        ThreadLocalRandom has
a _much_
>>>> better
>>>>>>  performance
>>>>>>>>   on
>>>>>>>>>>    servers! Or we
>>>>>>>>>>>>     just
>>>>>>>>>>>>>      use a simple XORShift which is
surely
>>>> good
>>>>>>  enough for
>>>>>>>>   most cases
>>>>>>>>>>    and
>>>>>>>>>>>>     performs
>>>>>>>>>>>>>      like hell. The spreading of XORShift
is
>>>> better
>>>>>>  than the
>>>>>>>>   standard
>>>>>>>>>>    Java
>>>>>>>>>>>>     algorithm
>>>>>>>>>>>>>      even ...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>      It could work.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>       d.) KeyFactory looks a bit
>>>> overengineered.
>>>>>>  The
>>>>>>>>   return type is
>>>>>>>>>>    either
>>>>>>>>>>>>>      Integer or byte [] but the encoded
>>>> value is
>>>>>>  always
>>>>>>>>   represented as
>>>>>>>>>>    String.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>      The key is encrypted and base64
>>>> encoded, but
>>>>>>  that's
>>>>>>>>   done in
>>>>>>>>>>>>>
>>>> HtmlResponseStateManager.writeViewStateField.
>>>>>>  The change
>>>>>>>>   only has
>>>>>>>>>>>>>      sense if we move the responsibility
of
>>>> encrypt
>>>>>>  to
>>>>>>>>>>>>>
>>>>>>  ServerSideStateCacheImpl/ClientSideStateCacheImpl. My
>>>>>>>>   opinion is
>>>>>>>>>>    it is
>>>>>>>>>>>>>      better to keep it as is.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>       e.) Instead of trashing
the
>>>> Session with
>>>>>>>>   setAttribute and
>>>>>>>>>>    synchronized
>>>>>>>>>>>>
>>>>>>>>>>>>>      blocks we should rather store
an
>>>> AtomicInteger.
>>>>>>  This is
>>>>>>>>   perfectly
>>>>>>>>>>    fine now
>>>>>>>>>>>>     as we
>>>>>>>>>>>>>      do not support java 1.4 any longer,
>>>> right?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>      The synchronized blocks are over
the
>>>>>>>>   SerializedViewCollection,
>>>>>>>>>>    which
>>>>>>>>>>>>>      is unique per session, so it
does not
>>>> suppose
>>>>>>  any
>>>>>>>>   overhead (tested
>>>>>>>>>>>>>      with YourKit profiler). But performance
>>>> tests
>>>>>>  shows that
>>>>>>>>   the
>>>>>>>>>>>>>      additional calls to session map
impose
>>>> an small
>>>>>>  overhead
>>>>>>>>   (the
>>>>>>>>>>    ideal is
>>>>>>>>>>>>>      minimize those calls). Since
the
>>>> counter is
>>>>>>  stored per
>>>>>>>>   session,
>>>>>>>>>>    there
>>>>>>>>>>>>>      is no chance of key clashing.
One
>>>> option could
>>>>>>  be
>>>>>>>>   something like
>>>>>>>>>>    the
>>>>>>>>>>>>>      trick used in Flash Scope. An
>>>> AtomicLong from a
>>>>>>  random
>>>>>>>>   seed, but
>>>>>>>>>>>>>      anyway it is necessary to change
the
>>>> algorithm
>>>>>>  for check
>>>>>>>>   if there
>>>>>>>>>>    is a
>>>>>>>>>>>>>      key clashing, generates another
key.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>       Just a few random ideas...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>      MS>> another one:
>>>>>>>>>>>>>      MS>> All that stuff has
nothing
>>>> to do with
>>>>>>  the
>>>>>>>>   RenderKit and
>>>>>>>>>>    should
>>>>>>>>>>>>     go
>>>>>>>>>>>>>      into
>>>> org.apache.myfaces.application.viewstate.
>>>>>>>>>>>>>      MS>> wdyt?
>>>>>>>>>>>>>
>>>>>>>>>>>>>      That stuff is used by
>>>>>>>>>>>>>
>>>>>>>>   org.apache.myfaces.renderkit.html.HtmlResponseStateManager,
so
>>>> in
>>>>>>>>>>>>>      theory is related to the renderkit
>>>> (because
>>>>>>>>   ResponseStateManager
>>>>>>>>>>    is
>>>>>>>>>>>>>      renderkit dependant), but it
is an
>>>> application
>>>>>>  scope
>>>>>>>>   concept.
>>>>>>>>>>    Maybe
>>>>>>>>>>>>>
>>>>>>  org.apache.myfaces.renderkit.application.viewstate. has
>>>>>>>>   more sense
>>>>>>>>>>    to
>>>>>>>>>>>>>      me.
>>>>>>>>>>>>>
>>>>>>>>>>>>>      MS>> Also the following
classes
>>>> are
>>>>>>  related and
>>>>>>>>   should imo
>>>>>>>>>>    get moved
>>>>>>>>>>>>>      to this new package:
>>>>>>>>>>>>>      MS>> * StateCache
>>>>>>>>>>>>>      MS>> * StateCacheFactory
>>>>>>>>>>>>>
>>>>>>>>>>>>>      Yes, a better name
>>>>>>>>>>>>>
>>>>>>>>>>>>>      MS>> * StateManagerImpl
>>>>>>>>>>>>>
>>>>>>>>>>>>>      No, StateManagerImpl deals with
the
>>>> logic
>>>>>>  related to
>>>>>>>>   calculate the
>>>>>>>>>>>>>      state to a view, StateCache deals
with
>>>> the
>>>>>>  storage of the
>>>>>>>>   view
>>>>>>>>>>    (maybe
>>>>>>>>>>>>>      a better name is StateStorage,
for
>>>> example
>>>>>>>>>>>>>
>>>>>>  ServerSideStateStorageImpl/ClientSideStateStorageImpl?
>>>>>>>>>>>>>
>>>>>>>>>>>>>      regards,
>>>>>>>>>>>>>
>>>>>>>>>>>>>      Leonardo
>>>>>>>>>>>>>
>>>>>>>>>>>>>>       LieGrue,
>>>>>>>>>>>>>>       strub
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>
>
>-- 
>Grant Smith - V.P. Information Technology
>Marathon Computer Systems, LLC.
>
>
>
>

Mime
View raw message