Uploaded image for project: 'Jackrabbit Oak'
  1. Jackrabbit Oak
  2. OAK-2875

Namespaces keep references to old node states

Details

    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 1.3.5, 1.4
    • core, jcr
    • None

    Description

      As described on the parent issue OA2849, the session namespaces keep a reference to a Tree instance which will make GC inefficient.

      Attachments

        1. OAK-2875-v1.patch
          12 kB
          Alex Deparvu
        2. OAK-2875-v2.patch
          13 kB
          Alex Deparvu

        Issue Links

          Activity

            stillalex Alex Deparvu added a comment -

            Linked to OAK-924, as a reference for the previous work on namespaces, especially wrt. performance considerations.

            stillalex Alex Deparvu added a comment - Linked to OAK-924 , as a reference for the previous work on namespaces, especially wrt. performance considerations.
            stillalex Alex Deparvu added a comment -

            WIP proposed patch, a much simpler version than what I had initially planned, to provide a preview for how this would look like.

            The idea is to have the namespaces bits react to the session refresh call and re-read the Tree thus releasing the old instances so GC can cleanup them up.

            here are a some aspects to be considered:

            • performance
            • GC
            • cleanly hooking up a refresh observer that will allow the namespaces class to react
            • JCR tck compliance
            stillalex Alex Deparvu added a comment - WIP proposed patch, a much simpler version than what I had initially planned, to provide a preview for how this would look like. The idea is to have the namespaces bits react to the session refresh call and re-read the Tree thus releasing the old instances so GC can cleanup them up. here are a some aspects to be considered: performance GC cleanly hooking up a refresh observer that will allow the namespaces class to react JCR tck compliance
            stillalex Alex Deparvu added a comment -

            refactored patch based on mduerig's offline feedback. I had to open the SessionNamespaces class so it can be accessed from the delegate package. Otherwise we might consider moving it to the delegate package directly.

            stillalex Alex Deparvu added a comment - refactored patch based on mduerig 's offline feedback. I had to open the SessionNamespaces class so it can be accessed from the delegate package. Otherwise we might consider moving it to the delegate package directly.

            Bulk move to 1.3.1

            edivad Davide Giannella added a comment - Bulk move to 1.3.1

            Bulk move to 1.3.2

            edivad Davide Giannella added a comment - Bulk move to 1.3.2

            Bulk move to 1.3.3.

            edivad Davide Giannella added a comment - Bulk move to 1.3.3.

            Bulk move to 1.3.4

            edivad Davide Giannella added a comment - Bulk move to 1.3.4
            stillalex Alex Deparvu added a comment -

            mduerig what do you think, can we work on cleaning & pushing this one to trunk?
            the only thing missing from the patch is bumping up the OSGi package export version on org.apache.jackrabbit.oak.namepath to 2.0

            stillalex Alex Deparvu added a comment - mduerig what do you think, can we work on cleaning & pushing this one to trunk? the only thing missing from the patch is bumping up the OSGi package export version on org.apache.jackrabbit.oak.namepath to 2.0

            Yes I think so. However I'd like a clearer picture on the benefits and implications:

            • is this related to OAK-2707? Can we quantify the perofmance impact?
            • is refreshing the name spaces spec. compliant? Backward compatible?
            • can we have a unit test covering the specific behaviour?
            mduerig Michael DĂĽrig added a comment - Yes I think so. However I'd like a clearer picture on the benefits and implications: is this related to OAK-2707 ? Can we quantify the perofmance impact? is refreshing the name spaces spec. compliant? Backward compatible? can we have a unit test covering the specific behaviour?
            stillalex Alex Deparvu added a comment -

            is refreshing the name spaces spec. compliant?

            right, this should be the main concern to driving this patch forward.

            as discussed with mduerig offline, this may hold the key to the spec compliance [0]:

            3.5.1 Namespace Registry
            
            The local namespace mapping of a session is determined by the initial set of mappings copied from the namespace registry and any session-local changes made to that set.
            

            So really the question is when is it legal to refresh the mappings, if ever?
            The patch I'm proposing would change the behavior from never refreshing to refreshing on session refresh.

            [0] http://www.day.com/specs/jcr/2.0/3_Repository_Model.html#3.5.1%20Namespace%20Registry

            stillalex Alex Deparvu added a comment - is refreshing the name spaces spec. compliant? right, this should be the main concern to driving this patch forward. as discussed with mduerig offline, this may hold the key to the spec compliance [0] : 3.5.1 Namespace Registry The local namespace mapping of a session is determined by the initial set of mappings copied from the namespace registry and any session-local changes made to that set. So really the question is when is it legal to refresh the mappings, if ever? The patch I'm proposing would change the behavior from never refreshing to refreshing on session refresh. [0] http://www.day.com/specs/jcr/2.0/3_Repository_Model.html#3.5.1%20Namespace%20Registry
            stillalex Alex Deparvu added a comment -

            The patch I'm proposing would change the behavior from never refreshing to refreshing on session refresh.

            coming back to this aspect. the original code uses Tree instances for the mappings, but the trees are live, so any change would be already reflected locally, my patch only grabs a new instance of the same tree (pointing to the same location) to release any potential hard nodestate reference. so as far as behavior goes, this change might not have such a high impact after all. mduerig thoughts?

            stillalex Alex Deparvu added a comment - The patch I'm proposing would change the behavior from never refreshing to refreshing on session refresh. coming back to this aspect. the original code uses Tree instances for the mappings, but the trees are live, so any change would be already reflected locally, my patch only grabs a new instance of the same tree (pointing to the same location) to release any potential hard nodestate reference. so as far as behavior goes, this change might not have such a high impact after all. mduerig thoughts?

            You are right. This didn't occur to me earlier. In this case +1 from me for the patch.

            mduerig Michael DĂĽrig added a comment - You are right. This didn't occur to me earlier. In this case +1 from me for the patch.
            stillalex Alex Deparvu added a comment - fixed with http://svn.apache.org/r1697423

            Bulk close for 1.3.5

            edivad Davide Giannella added a comment - Bulk close for 1.3.5

            People

              stillalex Alex Deparvu
              stillalex Alex Deparvu
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: