ODE
  1. ODE
  2. ODE-296

Support XSLT's document() function

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2
    • Component/s: BPEL Runtime
    • Labels:
      None

      Description

      Currently the BPEL runtime does not support executing XSL stylesheets that use the XSLT document() function to retrieve resources local to the process directory.

      1. AddDocumentSupport.patch
        5 kB
        Ciaran Jessup
      2. AddDocumentSupportv2.patch
        14 kB
        Ciaran Jessup
      3. FixUnneccessaryConstructorArgs.patch
        1 kB
        Ciaran Jessup
      4. HelloWorld2.zip
        8 kB
        Ciaran Jessup
      5. HelloWorld2.zip
        3 kB
        Ciaran Jessup

        Issue Links

          Activity

          Ciaran Jessup created issue -
          Hide
          Ciaran Jessup added a comment -

          This is an example test-case that uses the 'document()' syntax, that currently doesn't work.

          Show
          Ciaran Jessup added a comment - This is an example test-case that uses the 'document()' syntax, that currently doesn't work.
          Ciaran Jessup made changes -
          Field Original Value New Value
          Attachment HelloWorld2.zip [ 12382875 ]
          Hide
          Ciaran Jessup added a comment -

          Initial patch to provide document() support.

          Solves test-case, supports local relative to process folder filename resolution.

          • Requires further testing around:
            • Absolute paths
            • URLS
            • Local&Absolute file paths containing spaces.

          Should be pretty efficient the resources should only load in once per versioned process instance deployment, occasionally under heavy load might load in more than once, but mostly should be quite quick.

          Show
          Ciaran Jessup added a comment - Initial patch to provide document() support. Solves test-case, supports local relative to process folder filename resolution. Requires further testing around: Absolute paths URLS Local&Absolute file paths containing spaces. Should be pretty efficient the resources should only load in once per versioned process instance deployment, occasionally under heavy load might load in more than once, but mostly should be quite quick.
          Ciaran Jessup made changes -
          Attachment AddDocumentSupport.patch [ 12382876 ]
          Hide
          Matthieu Riou added a comment -

          I'm kind of reluctant to modify the OProcess. There are several reasons. First the OProcess is part of our serialized process model and we have to be careful about modifying it as it can make older versions incompatible with the new format. Second I'd like to avoid binding it to a specific location in the filesystem. The compiled representation of a process can be moved around and reused (think clustering for example).

          We actually have that location information even if it's not really immediate right now so you'll have to bear with me a little XslRuntimeUriResolver is created by the JaxpFunctionResolver and therefore could get information from the EvaluationContext that's already referenced by the function resolver. The EvaluationContext is implemented by ExprEvaluationContextImpl which references BpelRuntimeContext. BpelRuntimeContextImpl has a reference to BpelProcess that has a reference to ProcessConf. And ProcessConf has the getBaseUri() method you're looking for. A long way to follow

          So if you don't mind doing a bit of refactoring on your patch, it would be nice if you used what I just described to get the base URI instand of storing it on OProcess. You'll need to:

          • add a method on BpelRuntimeContextImpl and BpelRuntimeContext (like getBaseResourceURI() for example)
          • add the same method on EvaluationContext and ExprEvaluationContextImpl
          • pass the base URI to XslRuntimeUriResolver.

          Does that make sense? Thanks a lot for looking through this and for your patch!!

          Show
          Matthieu Riou added a comment - I'm kind of reluctant to modify the OProcess. There are several reasons. First the OProcess is part of our serialized process model and we have to be careful about modifying it as it can make older versions incompatible with the new format. Second I'd like to avoid binding it to a specific location in the filesystem. The compiled representation of a process can be moved around and reused (think clustering for example). We actually have that location information even if it's not really immediate right now so you'll have to bear with me a little XslRuntimeUriResolver is created by the JaxpFunctionResolver and therefore could get information from the EvaluationContext that's already referenced by the function resolver. The EvaluationContext is implemented by ExprEvaluationContextImpl which references BpelRuntimeContext. BpelRuntimeContextImpl has a reference to BpelProcess that has a reference to ProcessConf. And ProcessConf has the getBaseUri() method you're looking for. A long way to follow So if you don't mind doing a bit of refactoring on your patch, it would be nice if you used what I just described to get the base URI instand of storing it on OProcess. You'll need to: add a method on BpelRuntimeContextImpl and BpelRuntimeContext (like getBaseResourceURI() for example) add the same method on EvaluationContext and ExprEvaluationContextImpl pass the base URI to XslRuntimeUriResolver. Does that make sense? Thanks a lot for looking through this and for your patch!!
          Hide
          Ciaran Jessup added a comment - - edited

          Ok, this makes sense, I was unhappy with where I had put my changes anyway, as what you're describing pretty much matches what I understood the code to do. Do you have an issue with the 'resources' hashMap being on oProcess, I can't see it being any worse/better than the existing xslSheet field, and from a serialisation point of view it should just be initialised empty, although imho as perhaps with the xslSheets field it should be marked as transient ?

          Show
          Ciaran Jessup added a comment - - edited Ok, this makes sense, I was unhappy with where I had put my changes anyway, as what you're describing pretty much matches what I understood the code to do. Do you have an issue with the 'resources' hashMap being on oProcess, I can't see it being any worse/better than the existing xslSheet field, and from a serialisation point of view it should just be initialised empty, although imho as perhaps with the xslSheets field it should be marked as transient ?
          Hide
          Ciaran Jessup added a comment -

          Ammended approach taking into account the above comments. Removed the ProcessURI from the OProcess instance, however it still contains the set of resources, is this suitable?

          Show
          Ciaran Jessup added a comment - Ammended approach taking into account the above comments. Removed the ProcessURI from the OProcess instance, however it still contains the set of resources, is this suitable?
          Ciaran Jessup made changes -
          Attachment AddDocumentSupportv2.patch [ 12382912 ]
          Hide
          Matthieu Riou added a comment -

          Patch applied, thanks!

          I've actually removed the set of resources on OProcess. Really the OModel should be considered immutable after compilation, we shouldn't cache stuff in there. And I'm not sure the caching here is not a micro-optimization. Your OS will cache the file in memory after it's first read so subsequent reading should be really fast. Caching it again in the VM shouldn't make it much faster.

          I know, the XSL sheet itself is stored in the OProcess. But this was done when we didn't have the process store or a better way to access a filesystem resource at runtime, so that should actually be refactored at some point.

          Anyway, let me know if your experience is different and we'll find another way to cache those resources.

          And thanks again for your patch, it's most welcome!

          Show
          Matthieu Riou added a comment - Patch applied, thanks! I've actually removed the set of resources on OProcess. Really the OModel should be considered immutable after compilation, we shouldn't cache stuff in there. And I'm not sure the caching here is not a micro-optimization. Your OS will cache the file in memory after it's first read so subsequent reading should be really fast. Caching it again in the VM shouldn't make it much faster. I know, the XSL sheet itself is stored in the OProcess. But this was done when we didn't have the process store or a better way to access a filesystem resource at runtime, so that should actually be refactored at some point. Anyway, let me know if your experience is different and we'll find another way to cache those resources. And thanks again for your patch, it's most welcome!
          Hide
          Ciaran Jessup added a comment -

          Ok, thanks for applying the patch, I hit the file resources an awful lot, in fact for every web service invoke, one of my concerns was around being able to delete the deployment folder at runtime, on a loaded system unless the resources are cached I believe that if the file is continuously being 'read' then the file will be locked and I won't be able to get rid of that folder if I needed to, also, is there definately no con-current contention on the file-io ?

          Show
          Ciaran Jessup added a comment - Ok, thanks for applying the patch, I hit the file resources an awful lot, in fact for every web service invoke, one of my concerns was around being able to delete the deployment folder at runtime, on a loaded system unless the resources are cached I believe that if the file is continuously being 'read' then the file will be locked and I won't be able to get rid of that folder if I needed to, also, is there definately no con-current contention on the file-io ?
          Hide
          Ciaran Jessup added a comment -

          When looking over the code I noticed I was passing an instance variable that was already in scope, whilst I prefer to be explicit about dependencies, this patch removes my variable passing so it is more inline with the existing code

          Show
          Ciaran Jessup added a comment - When looking over the code I noticed I was passing an instance variable that was already in scope, whilst I prefer to be explicit about dependencies, this patch removes my variable passing so it is more inline with the existing code
          Ciaran Jessup made changes -
          Attachment FixUnneccessaryConstructorArgs.patch [ 12382971 ]
          Hide
          Ciaran Jessup added a comment -

          I'm not totally sure this is a micro-optimization, I use the document function a lot, each flow calls it many times. I've attached a simple example flow that uses document() to chose a an element in an external document. I've then loaded it from soapui to see what the behaviour is without any file-caching, and with (A simple static non-synchronised HashMap in the XslRuntimeUriResolver)

          The following documents my results (all times are in ms).:

          Single Thread, 1000 runs no caching:
          min ,max, avg
          8, 70, 9.93

          Four threads, 1000 runs no caching
          min ,max, avg
          7, 192, 25.13

          Single Thread, 1000 runs caching:
          min ,max, avg
          6, 54, 7.68,

          Four threads, 1000 runs caching
          min ,max, avg
          6, 88, 17.54

          Now I grant the times aren't massively different, but this example is trivial, in my real flows I can call this method tens of times, so shaving 20x 2ms off of one call can be quite a win for me !

          Looking at the code StreamUtils, it also seems to use a non FileChannel approach to loading in file data, In the past I've had quite a lot of success in leveraging NIO to increase the file-reading speeding, this might be worth looking at

          Show
          Ciaran Jessup added a comment - I'm not totally sure this is a micro-optimization, I use the document function a lot, each flow calls it many times. I've attached a simple example flow that uses document() to chose a an element in an external document. I've then loaded it from soapui to see what the behaviour is without any file-caching, and with (A simple static non-synchronised HashMap in the XslRuntimeUriResolver) The following documents my results (all times are in ms).: Single Thread, 1000 runs no caching: min ,max, avg 8, 70, 9.93 Four threads, 1000 runs no caching min ,max, avg 7, 192, 25.13 Single Thread, 1000 runs caching: min ,max, avg 6, 54, 7.68, Four threads, 1000 runs caching min ,max, avg 6, 88, 17.54 Now I grant the times aren't massively different, but this example is trivial, in my real flows I can call this method tens of times, so shaving 20x 2ms off of one call can be quite a win for me ! Looking at the code StreamUtils, it also seems to use a non FileChannel approach to loading in file data, In the past I've had quite a lot of success in leveraging NIO to increase the file-reading speeding, this might be worth looking at
          Ciaran Jessup made changes -
          Attachment HelloWorld2.zip [ 12383256 ]
          Hide
          Matthieu Riou added a comment -

          Closing the issue for now as the support has been implemented (thanks Ciaran!). If the performance gets too problematic, feel free to create another issue to track performance enhancements.

          Show
          Matthieu Riou added a comment - Closing the issue for now as the support has been implemented (thanks Ciaran!). If the performance gets too problematic, feel free to create another issue to track performance enhancements.
          Matthieu Riou made changes -
          Assignee Matthieu Riou [ mriou ]
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Tammo van Lessen made changes -
          Link This issue is related to ODE-904 [ ODE-904 ]

            People

            • Assignee:
              Matthieu Riou
              Reporter:
              Ciaran Jessup
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development