Re: [Axis2] Issues with the current code base
Hi Dims, Looking at uuid-001.png, AxisServlet.createAndSetInitialParamsToMsgCtxt is causing 5% of the total time which seems a lot for what I thought it was doing. I don't have a profiler to hand, could you show us a breakdown of what that method is calling and why it's taking so long? Cheers, David On 05/02/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Here's one more. Proliferation of getUUID's. http://ws.zones.apache.org/~dims/uuid-001.png http://ws.zones.apache.org/~dims/uuid-002.png We used to call getUUID once or at most twice. Now we create 6 uuid's for each req/resp taking up 1.9% (375/19533 * 100). Please change the code to create the correlation uuid's only when trace/debug flag is on. Otherwise, they should not be created. thanks, dims On 2/4/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Bill, I just started looking into the perf aspects of this checkin...Here's the first one. Please see the following 2 images: http://people.apache.org/~dims/msg-context-001.png http://people.apache.org/~dims/msg-context-002.png As you can plainly see, the servlet is called 2500 times, and takes a total time of 20360 ms . The method checkActivateWarning which is basically a no-op gets called 257,500 times and takes a total of 234 ms. Which is basically 1.14% of the total time taken to process the messages. Am sure you agree that checkActivateWarning not present in r495105 and was introduced later. So am going to get rid of it. thanks. thanks, dims On 1/29/07, Bill Nagy [EMAIL PROTECTED] wrote: Hi Sanjiva, On Mon, 2007-01-29 at 23:42 +0530, Sanjiva Weerawarana wrote: Bill, IMO I made a mistake in lifting my -1 on this patch .. this could've and should've gone in as an auxiliary bit of code without modifying MC at all. Calling mc.activateMessageContext on *every* message that comes in seems like a major ovehead to the mind even if not to the body (you know what I mean). There was no technical need whatsoever for this code to be in MC itself for Matt to be able to do whatever he needs to make Sandesha persist using Java object serialization instead of the serialization architecture that exists now. Next time I won't give in so easy :). You are certainly entitled to your opinion; I did attempt to continue the discussion. In any case, I'm yet to see an explanation from Anne for the algorithm used to figure out the parent service context etc. for a message context. Anne, can you explain how you're going about figuring those refs out please? Please don't say RTFS :(. I will make sure that she is aware of your request. We need to performance compare 1.1.1 with the current head and see what the impact of this change is. I would be more than happy for someone to compare r495105 to r495106(the version where the changes were committed) and would be more than willing to address any performance concerns that arise from that comparison. On code conventions- search the archives please .. we've had lots of discussion on this in the early days and decided on the conventions. I'm pretty sure we put them in the wiki somewhere but have no idea where off the top of my head :(. Interestingly, even the original JAX-WS proposal by IBM mentions coding conventions: http://wiki.apache.org/ws/FrontPage/Axis2/JAX-WS-Proposal. so maybe someone in IBM found a ref? I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. -Bill Sanjiva. On Mon, 2007-01-29 at 07:27 -0800, Bill Nagy wrote: Hi Deepal, On Mon, 2007-01-29 at 11:41 +0530, Deepal Jayasinghe wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? Ruchith : Do you want that for Security ? If none of you want this , who else need this ? As Matt pointed out, we went through this on an earlier discussion -- Sandesha is the intended consumer. I am asking this question b'coz introduction of MC serialization we have the following for each req. - When ever Axis2 received a message it calls activateMessageContext(msgContext); - And what that does is , it calls mc.activate(engineContext); method Unfortunately that method is TOO long
Re: [Axis2] Issues with the current code base
David, Latest run (fresh from svn), it's about 4%...It's the getUUID's and string concat to make the correlation id's. that's what i was talking about :) http://ws.zones.apache.org/~dims/msg-context-003.png -- dims On 2/5/07, David Illsley [EMAIL PROTECTED] wrote: Hi Dims, Looking at uuid-001.png, AxisServlet.createAndSetInitialParamsToMsgCtxt is causing 5% of the total time which seems a lot for what I thought it was doing. I don't have a profiler to hand, could you show us a breakdown of what that method is calling and why it's taking so long? Cheers, David On 05/02/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Here's one more. Proliferation of getUUID's. http://ws.zones.apache.org/~dims/uuid-001.png http://ws.zones.apache.org/~dims/uuid-002.png We used to call getUUID once or at most twice. Now we create 6 uuid's for each req/resp taking up 1.9% (375/19533 * 100). Please change the code to create the correlation uuid's only when trace/debug flag is on. Otherwise, they should not be created. thanks, dims On 2/4/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Bill, I just started looking into the perf aspects of this checkin...Here's the first one. Please see the following 2 images: http://people.apache.org/~dims/msg-context-001.png http://people.apache.org/~dims/msg-context-002.png As you can plainly see, the servlet is called 2500 times, and takes a total time of 20360 ms . The method checkActivateWarning which is basically a no-op gets called 257,500 times and takes a total of 234 ms. Which is basically 1.14% of the total time taken to process the messages. Am sure you agree that checkActivateWarning not present in r495105 and was introduced later. So am going to get rid of it. thanks. thanks, dims On 1/29/07, Bill Nagy [EMAIL PROTECTED] wrote: Hi Sanjiva, On Mon, 2007-01-29 at 23:42 +0530, Sanjiva Weerawarana wrote: Bill, IMO I made a mistake in lifting my -1 on this patch .. this could've and should've gone in as an auxiliary bit of code without modifying MC at all. Calling mc.activateMessageContext on *every* message that comes in seems like a major ovehead to the mind even if not to the body (you know what I mean). There was no technical need whatsoever for this code to be in MC itself for Matt to be able to do whatever he needs to make Sandesha persist using Java object serialization instead of the serialization architecture that exists now. Next time I won't give in so easy :). You are certainly entitled to your opinion; I did attempt to continue the discussion. In any case, I'm yet to see an explanation from Anne for the algorithm used to figure out the parent service context etc. for a message context. Anne, can you explain how you're going about figuring those refs out please? Please don't say RTFS :(. I will make sure that she is aware of your request. We need to performance compare 1.1.1 with the current head and see what the impact of this change is. I would be more than happy for someone to compare r495105 to r495106(the version where the changes were committed) and would be more than willing to address any performance concerns that arise from that comparison. On code conventions- search the archives please .. we've had lots of discussion on this in the early days and decided on the conventions. I'm pretty sure we put them in the wiki somewhere but have no idea where off the top of my head :(. Interestingly, even the original JAX-WS proposal by IBM mentions coding conventions: http://wiki.apache.org/ws/FrontPage/Axis2/JAX-WS-Proposal. so maybe someone in IBM found a ref? I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. -Bill Sanjiva. On Mon, 2007-01-29 at 07:27 -0800, Bill Nagy wrote: Hi Deepal, On Mon, 2007-01-29 at 11:41 +0530, Deepal Jayasinghe wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? Ruchith : Do you want that for Security ? If none of you want this , who else need this ? As Matt pointed out, we went through this on an earlier discussion --
Re: [Axis2] Issues with the current code base
On Sun, 2007-02-04 at 17:43 -0500, Davanum Srinivas wrote: Bill, I just started looking into the perf aspects of this checkin...Here's the first one. Please see the following 2 images: http://people.apache.org/~dims/msg-context-001.png http://people.apache.org/~dims/msg-context-002.png As you can plainly see, the servlet is called 2500 times, and takes a total time of 20360 ms . The method checkActivateWarning which is basically a no-op gets called 257,500 times and takes a total of 234 ms. Which is basically 1.14% of the total time taken to process the messages. Am sure you agree that checkActivateWarning not present in r495105 and was introduced later. So am going to get rid of it. thanks. +1. Bill/Ann, I'd like to offer to withdraw my request to make message context de-serialization work right .. can you consider going back to the original state where there was no attempt to come back correctly from serialization? IIRC all of these extra things were added to try to recover a correct context hierarchy and I'm not convinced it'll work anyway. Plus its a heavy overhead. I believe removing it does not affect your requirements for MC serialization right? (It was put in because I complained after all.) Sanjiva. -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Sanjiva, Hang on!. Am not done reviewing/fixing yet :) need another day or so. thanks, dims On 2/5/07, Sanjiva Weerawarana [EMAIL PROTECTED] wrote: On Sun, 2007-02-04 at 17:43 -0500, Davanum Srinivas wrote: Bill, I just started looking into the perf aspects of this checkin...Here's the first one. Please see the following 2 images: http://people.apache.org/~dims/msg-context-001.png http://people.apache.org/~dims/msg-context-002.png As you can plainly see, the servlet is called 2500 times, and takes a total time of 20360 ms . The method checkActivateWarning which is basically a no-op gets called 257,500 times and takes a total of 234 ms. Which is basically 1.14% of the total time taken to process the messages. Am sure you agree that checkActivateWarning not present in r495105 and was introduced later. So am going to get rid of it. thanks. +1. Bill/Ann, I'd like to offer to withdraw my request to make message context de-serialization work right .. can you consider going back to the original state where there was no attempt to come back correctly from serialization? IIRC all of these extra things were added to try to recover a correct context hierarchy and I'm not convinced it'll work anyway. Plus its a heavy overhead. I believe removing it does not affect your requirements for MC serialization right? (It was put in because I complained after all.) Sanjiva. -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Davanum Srinivas :: http://wso2.org/ :: Oxygen for Web Services Developers - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
I know what you were worried about... FWIW, I'm worried about the UUID thing as well. I'm also worried about the (now) 4% taken setting up transport information when it's really not clear what's CPU intensive in there (or that it should be). David On 05/02/07, Davanum Srinivas [EMAIL PROTECTED] wrote: David, Latest run (fresh from svn), it's about 4%...It's the getUUID's and string concat to make the correlation id's. that's what i was talking about :) http://ws.zones.apache.org/~dims/msg-context-003.png -- dims On 2/5/07, David Illsley [EMAIL PROTECTED] wrote: Hi Dims, Looking at uuid-001.png, AxisServlet.createAndSetInitialParamsToMsgCtxt is causing 5% of the total time which seems a lot for what I thought it was doing. I don't have a profiler to hand, could you show us a breakdown of what that method is calling and why it's taking so long? Cheers, David On 05/02/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Here's one more. Proliferation of getUUID's. http://ws.zones.apache.org/~dims/uuid-001.png http://ws.zones.apache.org/~dims/uuid-002.png We used to call getUUID once or at most twice. Now we create 6 uuid's for each req/resp taking up 1.9% (375/19533 * 100). Please change the code to create the correlation uuid's only when trace/debug flag is on. Otherwise, they should not be created. thanks, dims On 2/4/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Bill, I just started looking into the perf aspects of this checkin...Here's the first one. Please see the following 2 images: http://people.apache.org/~dims/msg-context-001.png http://people.apache.org/~dims/msg-context-002.png As you can plainly see, the servlet is called 2500 times, and takes a total time of 20360 ms . The method checkActivateWarning which is basically a no-op gets called 257,500 times and takes a total of 234 ms. Which is basically 1.14% of the total time taken to process the messages. Am sure you agree that checkActivateWarning not present in r495105 and was introduced later. So am going to get rid of it. thanks. thanks, dims On 1/29/07, Bill Nagy [EMAIL PROTECTED] wrote: Hi Sanjiva, On Mon, 2007-01-29 at 23:42 +0530, Sanjiva Weerawarana wrote: Bill, IMO I made a mistake in lifting my -1 on this patch .. this could've and should've gone in as an auxiliary bit of code without modifying MC at all. Calling mc.activateMessageContext on *every* message that comes in seems like a major ovehead to the mind even if not to the body (you know what I mean). There was no technical need whatsoever for this code to be in MC itself for Matt to be able to do whatever he needs to make Sandesha persist using Java object serialization instead of the serialization architecture that exists now. Next time I won't give in so easy :). You are certainly entitled to your opinion; I did attempt to continue the discussion. In any case, I'm yet to see an explanation from Anne for the algorithm used to figure out the parent service context etc. for a message context. Anne, can you explain how you're going about figuring those refs out please? Please don't say RTFS :(. I will make sure that she is aware of your request. We need to performance compare 1.1.1 with the current head and see what the impact of this change is. I would be more than happy for someone to compare r495105 to r495106(the version where the changes were committed) and would be more than willing to address any performance concerns that arise from that comparison. On code conventions- search the archives please .. we've had lots of discussion on this in the early days and decided on the conventions. I'm pretty sure we put them in the wiki somewhere but have no idea where off the top of my head :(. Interestingly, even the original JAX-WS proposal by IBM mentions coding conventions: http://wiki.apache.org/ws/FrontPage/Axis2/JAX-WS-Proposal. so maybe someone in IBM found a ref? I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. -Bill Sanjiva. On Mon, 2007-01-29 at 07:27 -0800, Bill Nagy wrote: Hi Deepal, On Mon, 2007-01-29 at 11:41 +0530, Deepal Jayasinghe wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to
Re: [Axis2] Issues with the current code base
Eliminated the uuid overhead (create them only when needed). We're down to 2%. Out of which 1% is to get the remote address from the servlet engine. http://ws.zones.apache.org/~dims/msg-context-004.png thanks, dims On 2/5/07, David Illsley [EMAIL PROTECTED] wrote: I know what you were worried about... FWIW, I'm worried about the UUID thing as well. I'm also worried about the (now) 4% taken setting up transport information when it's really not clear what's CPU intensive in there (or that it should be). David On 05/02/07, Davanum Srinivas [EMAIL PROTECTED] wrote: David, Latest run (fresh from svn), it's about 4%...It's the getUUID's and string concat to make the correlation id's. that's what i was talking about :) http://ws.zones.apache.org/~dims/msg-context-003.png -- dims On 2/5/07, David Illsley [EMAIL PROTECTED] wrote: Hi Dims, Looking at uuid-001.png, AxisServlet.createAndSetInitialParamsToMsgCtxt is causing 5% of the total time which seems a lot for what I thought it was doing. I don't have a profiler to hand, could you show us a breakdown of what that method is calling and why it's taking so long? Cheers, David On 05/02/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Here's one more. Proliferation of getUUID's. http://ws.zones.apache.org/~dims/uuid-001.png http://ws.zones.apache.org/~dims/uuid-002.png We used to call getUUID once or at most twice. Now we create 6 uuid's for each req/resp taking up 1.9% (375/19533 * 100). Please change the code to create the correlation uuid's only when trace/debug flag is on. Otherwise, they should not be created. thanks, dims On 2/4/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Bill, I just started looking into the perf aspects of this checkin...Here's the first one. Please see the following 2 images: http://people.apache.org/~dims/msg-context-001.png http://people.apache.org/~dims/msg-context-002.png As you can plainly see, the servlet is called 2500 times, and takes a total time of 20360 ms . The method checkActivateWarning which is basically a no-op gets called 257,500 times and takes a total of 234 ms. Which is basically 1.14% of the total time taken to process the messages. Am sure you agree that checkActivateWarning not present in r495105 and was introduced later. So am going to get rid of it. thanks. thanks, dims On 1/29/07, Bill Nagy [EMAIL PROTECTED] wrote: Hi Sanjiva, On Mon, 2007-01-29 at 23:42 +0530, Sanjiva Weerawarana wrote: Bill, IMO I made a mistake in lifting my -1 on this patch .. this could've and should've gone in as an auxiliary bit of code without modifying MC at all. Calling mc.activateMessageContext on *every* message that comes in seems like a major ovehead to the mind even if not to the body (you know what I mean). There was no technical need whatsoever for this code to be in MC itself for Matt to be able to do whatever he needs to make Sandesha persist using Java object serialization instead of the serialization architecture that exists now. Next time I won't give in so easy :). You are certainly entitled to your opinion; I did attempt to continue the discussion. In any case, I'm yet to see an explanation from Anne for the algorithm used to figure out the parent service context etc. for a message context. Anne, can you explain how you're going about figuring those refs out please? Please don't say RTFS :(. I will make sure that she is aware of your request. We need to performance compare 1.1.1 with the current head and see what the impact of this change is. I would be more than happy for someone to compare r495105 to r495106(the version where the changes were committed) and would be more than willing to address any performance concerns that arise from that comparison. On code conventions- search the archives please .. we've had lots of discussion on this in the early days and decided on the conventions. I'm pretty sure we put them in the wiki somewhere but have no idea where off the top of my head :(. Interestingly, even the original JAX-WS proposal by IBM mentions coding conventions: http://wiki.apache.org/ws/FrontPage/Axis2/JAX-WS-Proposal. so maybe someone in IBM found a ref? I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. -Bill
Re: [Axis2] Issues with the current code base
+1 .. how about doing this just *before* cutting the release branch? That way if the reformat causes issues it'll get fixed on the branch (and then merged). Furthermore, when the branch is merged later it will not appear to change everything .. as a reformat may. Sanjiva. On Fri, 2007-02-02 at 10:58 -0500, Glen Daniels wrote: I think we should also have periodic sweeps where someone uses their IDE or another tool to do a wholesale reformat of the entire codebase to match the conventions (this won't fix everything, no, but it can help), and then checks in ONLY the formatting changes. Maybe right before releases? --Glen Davanum Srinivas wrote: Bill, The same IDE that lets you find things easily, lets you reformat the code to sun convention too and we have some additional stuff on top of that which is also usually configurable. Definitely, there is no excuse for not following the conventions. Yes, if you see something wrong in someone's checkin not following the convention, you have to let them know immediately. Please don't wait 3 months and say there were 100 commits that did not follow the convention :) thanks, dims On 2/2/07, Sanjiva Weerawarana [EMAIL PROTECTED] wrote: On Thu, 2007-02-01 at 09:57 -0800, Bill Nagy wrote: I don't honestly think that anyone who we want to be writing code for the project would have trouble understanding code simply because braces weren't on the lines where they typically put them or that methods aren't named in the same way that they usually name them. With respect to this particular issue, I don't think that the vast majority of people go look for declarations by scrolling to the top of the file -- IDEs have made that unnecessary. I think that we simply disagree on where to draw the line for reasonable deviations from the rules. The whole point of having coding conventions is to get everyone to follow them. This is not negotiable- we have agreed on conventions and everyone should follow them. Why is that so hard? Its good for everyone and its a simple thing to do. We didn't pick the conventions I liked either but it doesn't matter .. that's what was decided on for the project and we must follow them. I completely agree. The issues that have been raised so far, however, have all been of the form The code gets invoked on every invocation -- I don't like that. I was addressing those issues. I committed the Those issues clearly have a performance impact! code, therefore I am ultimately responsible for it. Before I did so, I read through it and did my best to make sure that it was not adversely effecting performance, and those are the points that I keep raising. Is this a lot of code that was changed/added? Certainly. Do I believe that it is isolated when not in use? Yes. I don't have to run performance tests simply because somebody says so when I'm comfortable with the logic of a particular change. Likewise, I don't believe that anybody runs performance tests for the vast majority of the changes that they make to the code, and they probably have thought about isolation a lot less than was done so for this particular change. Fair enough. Let's wait till we have some numbers .. if there's no adverse impact then great. Sanjiva. -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Bill, I just started looking into the perf aspects of this checkin...Here's the first one. Please see the following 2 images: http://people.apache.org/~dims/msg-context-001.png http://people.apache.org/~dims/msg-context-002.png As you can plainly see, the servlet is called 2500 times, and takes a total time of 20360 ms . The method checkActivateWarning which is basically a no-op gets called 257,500 times and takes a total of 234 ms. Which is basically 1.14% of the total time taken to process the messages. Am sure you agree that checkActivateWarning not present in r495105 and was introduced later. So am going to get rid of it. thanks. thanks, dims On 1/29/07, Bill Nagy [EMAIL PROTECTED] wrote: Hi Sanjiva, On Mon, 2007-01-29 at 23:42 +0530, Sanjiva Weerawarana wrote: Bill, IMO I made a mistake in lifting my -1 on this patch .. this could've and should've gone in as an auxiliary bit of code without modifying MC at all. Calling mc.activateMessageContext on *every* message that comes in seems like a major ovehead to the mind even if not to the body (you know what I mean). There was no technical need whatsoever for this code to be in MC itself for Matt to be able to do whatever he needs to make Sandesha persist using Java object serialization instead of the serialization architecture that exists now. Next time I won't give in so easy :). You are certainly entitled to your opinion; I did attempt to continue the discussion. In any case, I'm yet to see an explanation from Anne for the algorithm used to figure out the parent service context etc. for a message context. Anne, can you explain how you're going about figuring those refs out please? Please don't say RTFS :(. I will make sure that she is aware of your request. We need to performance compare 1.1.1 with the current head and see what the impact of this change is. I would be more than happy for someone to compare r495105 to r495106(the version where the changes were committed) and would be more than willing to address any performance concerns that arise from that comparison. On code conventions- search the archives please .. we've had lots of discussion on this in the early days and decided on the conventions. I'm pretty sure we put them in the wiki somewhere but have no idea where off the top of my head :(. Interestingly, even the original JAX-WS proposal by IBM mentions coding conventions: http://wiki.apache.org/ws/FrontPage/Axis2/JAX-WS-Proposal. so maybe someone in IBM found a ref? I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. -Bill Sanjiva. On Mon, 2007-01-29 at 07:27 -0800, Bill Nagy wrote: Hi Deepal, On Mon, 2007-01-29 at 11:41 +0530, Deepal Jayasinghe wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? Ruchith : Do you want that for Security ? If none of you want this , who else need this ? As Matt pointed out, we went through this on an earlier discussion -- Sandesha is the intended consumer. I am asking this question b'coz introduction of MC serialization we have the following for each req. - When ever Axis2 received a message it calls activateMessageContext(msgContext); - And what that does is , it calls mc.activate(engineContext); method Unfortunately that method is TOO long and was very difficult to understand:). Ann can you simplify the method (that will be very helpful) . Actually, if you notice, the first line of MessageContext.activate(...) is a short-circuit test on needsToBeReconciled, which is only ever set when the MessageContext (and related parts) are deserialized using the MessageContext deserialization routines -- for all other cases, it ends up being a no-op so you can stop reading at that point 8-]. As for the method being too long, of the 306 lines in that method, 110 are comments/log messages, and the rest of the code consists of if-not-null-invoke-a-method blocks. Unfortunately the MessageContext has a lot of handles to other objects, so there need to be a number of those tests. If you're having difficulty understanding a particular section of that method, please ask and we'd be happy to explain it to you. I certainly think that the rest of the code could use some code bloat (i.e.
Re: [Axis2] Issues with the current code base
Here's one more. Proliferation of getUUID's. http://ws.zones.apache.org/~dims/uuid-001.png http://ws.zones.apache.org/~dims/uuid-002.png We used to call getUUID once or at most twice. Now we create 6 uuid's for each req/resp taking up 1.9% (375/19533 * 100). Please change the code to create the correlation uuid's only when trace/debug flag is on. Otherwise, they should not be created. thanks, dims On 2/4/07, Davanum Srinivas [EMAIL PROTECTED] wrote: Bill, I just started looking into the perf aspects of this checkin...Here's the first one. Please see the following 2 images: http://people.apache.org/~dims/msg-context-001.png http://people.apache.org/~dims/msg-context-002.png As you can plainly see, the servlet is called 2500 times, and takes a total time of 20360 ms . The method checkActivateWarning which is basically a no-op gets called 257,500 times and takes a total of 234 ms. Which is basically 1.14% of the total time taken to process the messages. Am sure you agree that checkActivateWarning not present in r495105 and was introduced later. So am going to get rid of it. thanks. thanks, dims On 1/29/07, Bill Nagy [EMAIL PROTECTED] wrote: Hi Sanjiva, On Mon, 2007-01-29 at 23:42 +0530, Sanjiva Weerawarana wrote: Bill, IMO I made a mistake in lifting my -1 on this patch .. this could've and should've gone in as an auxiliary bit of code without modifying MC at all. Calling mc.activateMessageContext on *every* message that comes in seems like a major ovehead to the mind even if not to the body (you know what I mean). There was no technical need whatsoever for this code to be in MC itself for Matt to be able to do whatever he needs to make Sandesha persist using Java object serialization instead of the serialization architecture that exists now. Next time I won't give in so easy :). You are certainly entitled to your opinion; I did attempt to continue the discussion. In any case, I'm yet to see an explanation from Anne for the algorithm used to figure out the parent service context etc. for a message context. Anne, can you explain how you're going about figuring those refs out please? Please don't say RTFS :(. I will make sure that she is aware of your request. We need to performance compare 1.1.1 with the current head and see what the impact of this change is. I would be more than happy for someone to compare r495105 to r495106(the version where the changes were committed) and would be more than willing to address any performance concerns that arise from that comparison. On code conventions- search the archives please .. we've had lots of discussion on this in the early days and decided on the conventions. I'm pretty sure we put them in the wiki somewhere but have no idea where off the top of my head :(. Interestingly, even the original JAX-WS proposal by IBM mentions coding conventions: http://wiki.apache.org/ws/FrontPage/Axis2/JAX-WS-Proposal. so maybe someone in IBM found a ref? I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. -Bill Sanjiva. On Mon, 2007-01-29 at 07:27 -0800, Bill Nagy wrote: Hi Deepal, On Mon, 2007-01-29 at 11:41 +0530, Deepal Jayasinghe wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? Ruchith : Do you want that for Security ? If none of you want this , who else need this ? As Matt pointed out, we went through this on an earlier discussion -- Sandesha is the intended consumer. I am asking this question b'coz introduction of MC serialization we have the following for each req. - When ever Axis2 received a message it calls activateMessageContext(msgContext); - And what that does is , it calls mc.activate(engineContext); method Unfortunately that method is TOO long and was very difficult to understand:). Ann can you simplify the method (that will be very helpful) . Actually, if you notice, the first line of MessageContext.activate(...) is a short-circuit test on needsToBeReconciled, which is only ever set when the MessageContext (and related parts) are deserialized using the MessageContext deserialization routines -- for all other cases, it ends up being a no-op so you can stop reading at
Re: [Axis2] Issues with the current code base
On Thu, 2007-02-01 at 09:57 -0800, Bill Nagy wrote: I don't honestly think that anyone who we want to be writing code for the project would have trouble understanding code simply because braces weren't on the lines where they typically put them or that methods aren't named in the same way that they usually name them. With respect to this particular issue, I don't think that the vast majority of people go look for declarations by scrolling to the top of the file -- IDEs have made that unnecessary. I think that we simply disagree on where to draw the line for reasonable deviations from the rules. The whole point of having coding conventions is to get everyone to follow them. This is not negotiable- we have agreed on conventions and everyone should follow them. Why is that so hard? Its good for everyone and its a simple thing to do. We didn't pick the conventions I liked either but it doesn't matter .. that's what was decided on for the project and we must follow them. I completely agree. The issues that have been raised so far, however, have all been of the form The code gets invoked on every invocation -- I don't like that. I was addressing those issues. I committed the Those issues clearly have a performance impact! code, therefore I am ultimately responsible for it. Before I did so, I read through it and did my best to make sure that it was not adversely effecting performance, and those are the points that I keep raising. Is this a lot of code that was changed/added? Certainly. Do I believe that it is isolated when not in use? Yes. I don't have to run performance tests simply because somebody says so when I'm comfortable with the logic of a particular change. Likewise, I don't believe that anybody runs performance tests for the vast majority of the changes that they make to the code, and they probably have thought about isolation a lot less than was done so for this particular change. Fair enough. Let's wait till we have some numbers .. if there's no adverse impact then great. Sanjiva. -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Bill, The same IDE that lets you find things easily, lets you reformat the code to sun convention too and we have some additional stuff on top of that which is also usually configurable. Definitely, there is no excuse for not following the conventions. Yes, if you see something wrong in someone's checkin not following the convention, you have to let them know immediately. Please don't wait 3 months and say there were 100 commits that did not follow the convention :) thanks, dims On 2/2/07, Sanjiva Weerawarana [EMAIL PROTECTED] wrote: On Thu, 2007-02-01 at 09:57 -0800, Bill Nagy wrote: I don't honestly think that anyone who we want to be writing code for the project would have trouble understanding code simply because braces weren't on the lines where they typically put them or that methods aren't named in the same way that they usually name them. With respect to this particular issue, I don't think that the vast majority of people go look for declarations by scrolling to the top of the file -- IDEs have made that unnecessary. I think that we simply disagree on where to draw the line for reasonable deviations from the rules. The whole point of having coding conventions is to get everyone to follow them. This is not negotiable- we have agreed on conventions and everyone should follow them. Why is that so hard? Its good for everyone and its a simple thing to do. We didn't pick the conventions I liked either but it doesn't matter .. that's what was decided on for the project and we must follow them. I completely agree. The issues that have been raised so far, however, have all been of the form The code gets invoked on every invocation -- I don't like that. I was addressing those issues. I committed the Those issues clearly have a performance impact! code, therefore I am ultimately responsible for it. Before I did so, I read through it and did my best to make sure that it was not adversely effecting performance, and those are the points that I keep raising. Is this a lot of code that was changed/added? Certainly. Do I believe that it is isolated when not in use? Yes. I don't have to run performance tests simply because somebody says so when I'm comfortable with the logic of a particular change. Likewise, I don't believe that anybody runs performance tests for the vast majority of the changes that they make to the code, and they probably have thought about isolation a lot less than was done so for this particular change. Fair enough. Let's wait till we have some numbers .. if there's no adverse impact then great. Sanjiva. -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Davanum Srinivas :: http://wso2.org/ :: Oxygen for Web Services Developers - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
I think we should also have periodic sweeps where someone uses their IDE or another tool to do a wholesale reformat of the entire codebase to match the conventions (this won't fix everything, no, but it can help), and then checks in ONLY the formatting changes. Maybe right before releases? --Glen Davanum Srinivas wrote: Bill, The same IDE that lets you find things easily, lets you reformat the code to sun convention too and we have some additional stuff on top of that which is also usually configurable. Definitely, there is no excuse for not following the conventions. Yes, if you see something wrong in someone's checkin not following the convention, you have to let them know immediately. Please don't wait 3 months and say there were 100 commits that did not follow the convention :) thanks, dims On 2/2/07, Sanjiva Weerawarana [EMAIL PROTECTED] wrote: On Thu, 2007-02-01 at 09:57 -0800, Bill Nagy wrote: I don't honestly think that anyone who we want to be writing code for the project would have trouble understanding code simply because braces weren't on the lines where they typically put them or that methods aren't named in the same way that they usually name them. With respect to this particular issue, I don't think that the vast majority of people go look for declarations by scrolling to the top of the file -- IDEs have made that unnecessary. I think that we simply disagree on where to draw the line for reasonable deviations from the rules. The whole point of having coding conventions is to get everyone to follow them. This is not negotiable- we have agreed on conventions and everyone should follow them. Why is that so hard? Its good for everyone and its a simple thing to do. We didn't pick the conventions I liked either but it doesn't matter .. that's what was decided on for the project and we must follow them. I completely agree. The issues that have been raised so far, however, have all been of the form The code gets invoked on every invocation -- I don't like that. I was addressing those issues. I committed the Those issues clearly have a performance impact! code, therefore I am ultimately responsible for it. Before I did so, I read through it and did my best to make sure that it was not adversely effecting performance, and those are the points that I keep raising. Is this a lot of code that was changed/added? Certainly. Do I believe that it is isolated when not in use? Yes. I don't have to run performance tests simply because somebody says so when I'm comfortable with the logic of a particular change. Likewise, I don't believe that anybody runs performance tests for the vast majority of the changes that they make to the code, and they probably have thought about isolation a lot less than was done so for this particular change. Fair enough. Let's wait till we have some numbers .. if there's no adverse impact then great. Sanjiva. -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Hi Rajith,
Usage of the serialization is/will be configurable through Sandesha --
Matt said that he would be adding code to do that (if it isn't there
already.) If the serialization doesn't get invoked, then all that will
occur will be a few no-op method invocations.
I don't agree with point 5 being a definite no no. I believe that it
is a matter of personal preference and if the author of the code
believes that it makes it easier to read and doesn't create issues then
so be it. Any reasonable editor today will have no difficulty locating
the definition if you need to look at it.
As I said, I certainly welcome any concerned party to show me (in
hard/accurate numbers) where the code does not perform and it will be
addressed.
-Bill
On Wed, 2007-01-31 at 10:24 -0500, Rajith Attapattu wrote:
I do have one question for Anne, is this serialization configurable ?
In other words can I switch off this behaviour if I don't want it?
(Pardon me for not going through the code to figure this out )
I also share the same concerns as Deepal about code conventions.
No matter what coding convention was followed, point 5 is a definite
no no.
Sanjiva, I am glad that you are requesting some benchmarking, I was
consistently proposing that Anne should subject this code through some
high volume message scenarios to figure out the performance impact.
The basis for my objections was that the overhead introduced by this
feature, out weighs the benefits of it. YMMV
Rajith
On 1/29/07, Deepal Jayasinghe [EMAIL PROTECTED] wrote:
Hi Bill,
Among the all , the most worst thing I saw in the code is
following kind
of things, I strongly believe we should not have this kind
of code in
the code base, If you found such kind of code please point
out them then
and there.
- String tmpClassNameStr = null; (is this the way we
initialize to
NULL )
- String tmpHasList = no list
- Unnecessary casting
- A number of unused variables
- Variable declarations here and there (as an example
private static
final String - selfManagedDataDelimiter = *;)
I'm indifferent on the first two; in some cases it makes the
code easier
to read and debug at the cost of an assignment and space in
the string
table.
Well , more focus should be for code readability than
debugging .
The third one should be caught by any decent compiler and
eliminated (so long as you're not casting back and forth) and
again
sometimes enhances readability so I'm indifferent on this one
as well.
I agree on the fourth -- I don't think that there's ever a
good case for
extraneous variables. The fifth is again a code readability
issue and
one of the reasons that Java doesn't require that you declare
everything
up front.
Thank's for trying to clarify all these points. Just hope not
everybody
will start writing code like this :-)
BTW in point 5, I was talking about class level 'public
static'
variables, not method level variables.
For e.g.
class {
public static v1;
method1 ();
public static v2();
method2();
}
I dont think this is the way to go.
Deepal.
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
On Thu, 2007-02-01 at 06:10 -0800, Bill Nagy wrote: Hi Rajith, Usage of the serialization is/will be configurable through Sandesha -- Matt said that he would be adding code to do that (if it isn't there already.) If the serialization doesn't get invoked, then all that will occur will be a few no-op method invocations. I don't agree with point 5 being a definite no no. I believe that it is a matter of personal preference and if the author of the code believes that it makes it easier to read and doesn't create issues then so be it. Any reasonable editor today will have no difficulty locating the definition if you need to look at it. I disagree- as an open source effort we should write in ways that everyone can easily read and understand. These conventions are not tailorable for personal preference! It seems to me that most people declare variables at the top and methods below. Is that such a hard convention to accept? As I said, I certainly welcome any concerned party to show me (in hard/accurate numbers) where the code does not perform and it will be addressed. IMO its everyone's responsibility to make sure Axis2 performs well. Telling someone else to run perf tests, do profiling and then point out the place to fix is silly .. that person might as well fix it. Can't Ann write some tests and check the perf and confirm all is koshure?? I'd prefer to be having this conversation with Ann .. not that I don't like to chat with you Bill ;-)). Sanjiva. -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Sanjiva, IMO its everyone's responsibility to make sure Axis2 performs well. Telling someone else to run perf tests, do profiling and then point out the place to fix is silly .. that person might as well fix it. Can't Ann write some tests and check the perf and confirm all is koshure?? I'd prefer to be having this conversation with Ann .. not that I don't like to chat with you Bill ;-)). Sanjiva. Do you have some specific concerns about performance? Do you have any specific scenarios to consider? Ann WebSphere Development, Web Services Engine IBM Sanjiva Weerawarana [EMAIL PROTECTED] To ce.lkaxis-dev@ws.apache.org cc 02/01/2007 10:03 AMSubject Re: [Axis2] Issues with the current code base Please respond to [EMAIL PROTECTED] e.org On Thu, 2007-02-01 at 06:10 -0800, Bill Nagy wrote: Hi Rajith, Usage of the serialization is/will be configurable through Sandesha -- Matt said that he would be adding code to do that (if it isn't there already.) If the serialization doesn't get invoked, then all that will occur will be a few no-op method invocations. I don't agree with point 5 being a definite no no. I believe that it is a matter of personal preference and if the author of the code believes that it makes it easier to read and doesn't create issues then so be it. Any reasonable editor today will have no difficulty locating the definition if you need to look at it. I disagree- as an open source effort we should write in ways that everyone can easily read and understand. These conventions are not tailorable for personal preference! It seems to me that most people declare variables at the top and methods below. Is that such a hard convention to accept? As I said, I certainly welcome any concerned party to show me (in hard/accurate numbers) where the code does not perform and it will be addressed. IMO its everyone's responsibility to make sure Axis2 performs well. Telling someone else to run perf tests, do profiling and then point out the place to fix is silly .. that person might as well fix it. Can't Ann write some tests and check the perf and confirm all is koshure?? I'd prefer to be having this conversation with Ann .. not that I don't like to chat with you Bill ;-)). Sanjiva. -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Hi Sanjiva, On Thu, 2007-02-01 at 21:33 +0530, Sanjiva Weerawarana wrote: On Thu, 2007-02-01 at 06:10 -0800, Bill Nagy wrote: Hi Rajith, Usage of the serialization is/will be configurable through Sandesha -- Matt said that he would be adding code to do that (if it isn't there already.) If the serialization doesn't get invoked, then all that will occur will be a few no-op method invocations. I don't agree with point 5 being a definite no no. I believe that it is a matter of personal preference and if the author of the code believes that it makes it easier to read and doesn't create issues then so be it. Any reasonable editor today will have no difficulty locating the definition if you need to look at it. I disagree- as an open source effort we should write in ways that everyone can easily read and understand. These conventions are not tailorable for personal preference! It seems to me that most people declare variables at the top and methods below. Is that such a hard convention to accept? I don't honestly think that anyone who we want to be writing code for the project would have trouble understanding code simply because braces weren't on the lines where they typically put them or that methods aren't named in the same way that they usually name them. With respect to this particular issue, I don't think that the vast majority of people go look for declarations by scrolling to the top of the file -- IDEs have made that unnecessary. I think that we simply disagree on where to draw the line for reasonable deviations from the rules. As I said, I certainly welcome any concerned party to show me (in hard/accurate numbers) where the code does not perform and it will be addressed. IMO its everyone's responsibility to make sure Axis2 performs well. Telling someone else to run perf tests, do profiling and then point out the place to fix is silly .. that person might as well fix it. Can't Ann write some tests and check the perf and confirm all is koshure?? I completely agree. The issues that have been raised so far, however, have all been of the form The code gets invoked on every invocation -- I don't like that. I was addressing those issues. I committed the code, therefore I am ultimately responsible for it. Before I did so, I read through it and did my best to make sure that it was not adversely effecting performance, and those are the points that I keep raising. Is this a lot of code that was changed/added? Certainly. Do I believe that it is isolated when not in use? Yes. I don't have to run performance tests simply because somebody says so when I'm comfortable with the logic of a particular change. Likewise, I don't believe that anybody runs performance tests for the vast majority of the changes that they make to the code, and they probably have thought about isolation a lot less than was done so for this particular change. Now if you want to raise the issue of performance when this particular branch of code is in use, that's a fair thing to look at. Right from the start, however, I will tell you that what existed before and what exists in the new code are not functionally equivalent. I'm not saying that the new code is slower or that if it is that it shouldn't be made to be faster -- I'm simply saying that it won't be comparing apples to apples. I'd prefer to be having this conversation with Ann .. not that I don't like to chat with you Bill ;-)). That's fine; she's free to speak up (as she did.) I was just aware that she was busy, and since I'm aware of the details I wanted to address your concerns as quickly as possible. Also, as I stated, I committed the code into the tree, and so am ultimately responsible for that commit. -Bill - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
I do have one question for Anne, is this serialization configurable ?
In other words can I switch off this behaviour if I don't want it?
(Pardon me for not going through the code to figure this out )
I also share the same concerns as Deepal about code conventions.
No matter what coding convention was followed, point 5 is a definite no no.
Sanjiva, I am glad that you are requesting some benchmarking, I was
consistently proposing that Anne should subject this code through some high
volume message scenarios to figure out the performance impact.
The basis for my objections was that the overhead introduced by this
feature, out weighs the benefits of it. YMMV
Rajith
On 1/29/07, Deepal Jayasinghe [EMAIL PROTECTED] wrote:
Hi Bill,
Among the all , the most worst thing I saw in the code is following kind
of things, I strongly believe we should not have this kind of code in
the code base, If you found such kind of code please point out them then
and there.
- String tmpClassNameStr = null; (is this the way we initialize to
NULL )
- String tmpHasList = no list
- Unnecessary casting
- A number of unused variables
- Variable declarations here and there (as an example private static
final String - selfManagedDataDelimiter = *;)
I'm indifferent on the first two; in some cases it makes the code easier
to read and debug at the cost of an assignment and space in the string
table.
Well , more focus should be for code readability than debugging .
The third one should be caught by any decent compiler and
eliminated (so long as you're not casting back and forth) and again
sometimes enhances readability so I'm indifferent on this one as well.
I agree on the fourth -- I don't think that there's ever a good case for
extraneous variables. The fifth is again a code readability issue and
one of the reasons that Java doesn't require that you declare everything
up front.
Thank's for trying to clarify all these points. Just hope not everybody
will start writing code like this :-)
BTW in point 5, I was talking about class level 'public static'
variables, not method level variables.
For e.g.
class {
public static v1;
method1 ();
public static v2();
method2();
}
I dont think this is the way to go.
Deepal.
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Hi all, Deepal Jayasinghe [EMAIL PROTECTED] wrote on 29/01/2007 06:11:47: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? I'm working on a patch that will allow the Sandesha user to decide if they want to use the serialization code. I think there are good reasons to call it - see some of our mails on the Sandesha list, or keep an eye out for my patch. I hoped to get it out last week, but I've been quite busy - but it's high on my list! Matt - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Hi Deepal, On Mon, 2007-01-29 at 11:41 +0530, Deepal Jayasinghe wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? Ruchith : Do you want that for Security ? If none of you want this , who else need this ? As Matt pointed out, we went through this on an earlier discussion -- Sandesha is the intended consumer. I am asking this question b'coz introduction of MC serialization we have the following for each req. - When ever Axis2 received a message it calls activateMessageContext(msgContext); - And what that does is , it calls mc.activate(engineContext); method Unfortunately that method is TOO long and was very difficult to understand:). Ann can you simplify the method (that will be very helpful) . Actually, if you notice, the first line of MessageContext.activate(...) is a short-circuit test on needsToBeReconciled, which is only ever set when the MessageContext (and related parts) are deserialized using the MessageContext deserialization routines -- for all other cases, it ends up being a no-op so you can stop reading at that point 8-]. As for the method being too long, of the 306 lines in that method, 110 are comments/log messages, and the rest of the code consists of if-not-null-invoke-a-method blocks. Unfortunately the MessageContext has a lot of handles to other objects, so there need to be a number of those tests. If you're having difficulty understanding a particular section of that method, please ask and we'd be happy to explain it to you. I certainly think that the rest of the code could use some code bloat (i.e. comments) -- e.g. there are no class-level comments on ListenerManager, AxisConfiguration, PhaseResolver (just to name a few.) In the meantime code convention in MC has changed a lot and need to have very consistent code convention, please do not differ form that. This is the second time that code conventions have been mentioned (Sanjiva brought this up in an earlier discussion as well) -- I was not aware of these, and was unable to find anything in the docs that talk about them. (The Developer Guidelines only talk about the mailing list.) Could you please point me to where these are codified? Among the all , the most worst thing I saw in the code is following kind of things, I strongly believe we should not have this kind of code in the code base, If you found such kind of code please point out them then and there. - String tmpClassNameStr = null; (is this the way we initialize to NULL ) - String tmpHasList = no list - Unnecessary casting - A number of unused variables - Variable declarations here and there (as an example private static final String - selfManagedDataDelimiter = *;) I'm indifferent on the first two; in some cases it makes the code easier to read and debug at the cost of an assignment and space in the string table. The third one should be caught by any decent compiler and eliminated (so long as you're not casting back and forth) and again sometimes enhances readability so I'm indifferent on this one as well. I agree on the fourth -- I don't think that there's ever a good case for extraneous variables. The fifth is again a code readability issue and one of the reasons that Java doesn't require that you declare everything up front. -Bill - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Bill, IMO I made a mistake in lifting my -1 on this patch .. this could've and should've gone in as an auxiliary bit of code without modifying MC at all. Calling mc.activateMessageContext on *every* message that comes in seems like a major ovehead to the mind even if not to the body (you know what I mean). There was no technical need whatsoever for this code to be in MC itself for Matt to be able to do whatever he needs to make Sandesha persist using Java object serialization instead of the serialization architecture that exists now. Next time I won't give in so easy :). In any case, I'm yet to see an explanation from Anne for the algorithm used to figure out the parent service context etc. for a message context. Anne, can you explain how you're going about figuring those refs out please? Please don't say RTFS :(. We need to performance compare 1.1.1 with the current head and see what the impact of this change is. On code conventions- search the archives please .. we've had lots of discussion on this in the early days and decided on the conventions. I'm pretty sure we put them in the wiki somewhere but have no idea where off the top of my head :(. Interestingly, even the original JAX-WS proposal by IBM mentions coding conventions: http://wiki.apache.org/ws/FrontPage/Axis2/JAX-WS-Proposal. so maybe someone in IBM found a ref? Sanjiva. On Mon, 2007-01-29 at 07:27 -0800, Bill Nagy wrote: Hi Deepal, On Mon, 2007-01-29 at 11:41 +0530, Deepal Jayasinghe wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? Ruchith : Do you want that for Security ? If none of you want this , who else need this ? As Matt pointed out, we went through this on an earlier discussion -- Sandesha is the intended consumer. I am asking this question b'coz introduction of MC serialization we have the following for each req. - When ever Axis2 received a message it calls activateMessageContext(msgContext); - And what that does is , it calls mc.activate(engineContext); method Unfortunately that method is TOO long and was very difficult to understand:). Ann can you simplify the method (that will be very helpful) . Actually, if you notice, the first line of MessageContext.activate(...) is a short-circuit test on needsToBeReconciled, which is only ever set when the MessageContext (and related parts) are deserialized using the MessageContext deserialization routines -- for all other cases, it ends up being a no-op so you can stop reading at that point 8-]. As for the method being too long, of the 306 lines in that method, 110 are comments/log messages, and the rest of the code consists of if-not-null-invoke-a-method blocks. Unfortunately the MessageContext has a lot of handles to other objects, so there need to be a number of those tests. If you're having difficulty understanding a particular section of that method, please ask and we'd be happy to explain it to you. I certainly think that the rest of the code could use some code bloat (i.e. comments) -- e.g. there are no class-level comments on ListenerManager, AxisConfiguration, PhaseResolver (just to name a few.) In the meantime code convention in MC has changed a lot and need to have very consistent code convention, please do not differ form that. This is the second time that code conventions have been mentioned (Sanjiva brought this up in an earlier discussion as well) -- I was not aware of these, and was unable to find anything in the docs that talk about them. (The Developer Guidelines only talk about the mailing list.) Could you please point me to where these are codified? Among the all , the most worst thing I saw in the code is following kind of things, I strongly believe we should not have this kind of code in the code base, If you found such kind of code please point out them then and there. - String tmpClassNameStr = null; (is this the way we initialize to NULL ) - String tmpHasList = no list - Unnecessary casting - A number of unused variables - Variable declarations here and there (as an example private static final String - selfManagedDataDelimiter = *;) I'm indifferent on the first two; in some cases it makes the code easier to read and debug at the cost of an assignment and space in the string table. The third one should be caught by any decent compiler and eliminated (so long as you're not casting back and forth) and again sometimes enhances readability so I'm indifferent on this one as well. I agree on the fourth -- I don't think that there's ever a good case for extraneous variables. The fifth is
Re: [Axis2] Issues with the current code base
Hi Sanjiva, On Mon, 2007-01-29 at 23:42 +0530, Sanjiva Weerawarana wrote: Bill, IMO I made a mistake in lifting my -1 on this patch .. this could've and should've gone in as an auxiliary bit of code without modifying MC at all. Calling mc.activateMessageContext on *every* message that comes in seems like a major ovehead to the mind even if not to the body (you know what I mean). There was no technical need whatsoever for this code to be in MC itself for Matt to be able to do whatever he needs to make Sandesha persist using Java object serialization instead of the serialization architecture that exists now. Next time I won't give in so easy :). You are certainly entitled to your opinion; I did attempt to continue the discussion. In any case, I'm yet to see an explanation from Anne for the algorithm used to figure out the parent service context etc. for a message context. Anne, can you explain how you're going about figuring those refs out please? Please don't say RTFS :(. I will make sure that she is aware of your request. We need to performance compare 1.1.1 with the current head and see what the impact of this change is. I would be more than happy for someone to compare r495105 to r495106(the version where the changes were committed) and would be more than willing to address any performance concerns that arise from that comparison. On code conventions- search the archives please .. we've had lots of discussion on this in the early days and decided on the conventions. I'm pretty sure we put them in the wiki somewhere but have no idea where off the top of my head :(. Interestingly, even the original JAX-WS proposal by IBM mentions coding conventions: http://wiki.apache.org/ws/FrontPage/Axis2/JAX-WS-Proposal. so maybe someone in IBM found a ref? I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. -Bill Sanjiva. On Mon, 2007-01-29 at 07:27 -0800, Bill Nagy wrote: Hi Deepal, On Mon, 2007-01-29 at 11:41 +0530, Deepal Jayasinghe wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? Ruchith : Do you want that for Security ? If none of you want this , who else need this ? As Matt pointed out, we went through this on an earlier discussion -- Sandesha is the intended consumer. I am asking this question b'coz introduction of MC serialization we have the following for each req. - When ever Axis2 received a message it calls activateMessageContext(msgContext); - And what that does is , it calls mc.activate(engineContext); method Unfortunately that method is TOO long and was very difficult to understand:). Ann can you simplify the method (that will be very helpful) . Actually, if you notice, the first line of MessageContext.activate(...) is a short-circuit test on needsToBeReconciled, which is only ever set when the MessageContext (and related parts) are deserialized using the MessageContext deserialization routines -- for all other cases, it ends up being a no-op so you can stop reading at that point 8-]. As for the method being too long, of the 306 lines in that method, 110 are comments/log messages, and the rest of the code consists of if-not-null-invoke-a-method blocks. Unfortunately the MessageContext has a lot of handles to other objects, so there need to be a number of those tests. If you're having difficulty understanding a particular section of that method, please ask and we'd be happy to explain it to you. I certainly think that the rest of the code could use some code bloat (i.e. comments) -- e.g. there are no class-level comments on ListenerManager, AxisConfiguration, PhaseResolver (just to name a few.) In the meantime code convention in MC has changed a lot and need to have very consistent code convention, please do not differ form that. This is the second time that code conventions have been mentioned (Sanjiva brought this up in an earlier discussion as well) -- I was not aware of these, and was unable to find anything in the docs that talk about them. (The Developer Guidelines only talk about the mailing list.) Could you please point me to where these are codified? Among the all , the most worst thing I saw in the code is
coding conventions (was: Re: [Axis2] Issues with the current code base)
On Mon, 2007-01-29 at 13:37 -0800, Bill Nagy wrote: I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. Absolutely; fair enough. The basic rule was that we're following the Sun Java coding conventions (http://java.sun.com/docs/codeconv/) except for the number of columns used. I argued hard for 80 col but was voted down .. we settled at 100, which was the middle between 80 and 120 ;(. Does anyone need more details? If not please conform. (Old timers: if you remember the thread and can give a ref that maybe useful.) If someone volunteers to put this in the Wiki in big bold letters that'll be great. Thanks Bill for pushing this to closure! Sanjiva. p.s.: Don't take this to mean that only new folks are guilty of violating the conventions .. everyone is :(. But we need to try to do better .. and its easier to get folks to get in line as they start rather than later. Still, if you find offenders please jump on them! (In a nice warm, fuzzy kinda way ;-)) -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
Hi Bill,
Among the all , the most worst thing I saw in the code is following kind
of things, I strongly believe we should not have this kind of code in
the code base, If you found such kind of code please point out them then
and there.
- String tmpClassNameStr = null; (is this the way we initialize to
NULL )
- String tmpHasList = no list
- Unnecessary casting
- A number of unused variables
- Variable declarations here and there (as an example private static
final String - selfManagedDataDelimiter = *;)
I'm indifferent on the first two; in some cases it makes the code easier
to read and debug at the cost of an assignment and space in the string
table.
Well , more focus should be for code readability than debugging .
The third one should be caught by any decent compiler and
eliminated (so long as you're not casting back and forth) and again
sometimes enhances readability so I'm indifferent on this one as well.
I agree on the fourth -- I don't think that there's ever a good case for
extraneous variables. The fifth is again a code readability issue and
one of the reasons that Java doesn't require that you declare everything
up front.
Thank's for trying to clarify all these points. Just hope not everybody
will start writing code like this :-)
BTW in point 5, I was talking about class level 'public static'
variables, not method level variables.
For e.g.
class {
public static v1;
method1 ();
public static v2();
method2();
}
I dont think this is the way to go.
Deepal.
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: coding conventions (was: Re: [Axis2] Issues with the current code base)
FYI : http://wiki.apache.org/ws/FrontPage/Axis2/CodeQuality Thanks, Ruchith On 1/30/07, Sanjiva Weerawarana [EMAIL PROTECTED] wrote: On Mon, 2007-01-29 at 13:37 -0800, Bill Nagy wrote: I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. Absolutely; fair enough. The basic rule was that we're following the Sun Java coding conventions (http://java.sun.com/docs/codeconv/) except for the number of columns used. I argued hard for 80 col but was voted down .. we settled at 100, which was the middle between 80 and 120 ;(. Does anyone need more details? If not please conform. (Old timers: if you remember the thread and can give a ref that maybe useful.) If someone volunteers to put this in the Wiki in big bold letters that'll be great. Thanks Bill for pushing this to closure! Sanjiva. p.s.: Don't take this to mean that only new folks are guilty of violating the conventions .. everyone is :(. But we need to try to do better .. and its easier to get folks to get in line as they start rather than later. Still, if you find offenders please jump on them! (In a nice warm, fuzzy kinda way ;-)) -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- www.ruchith.org www.wso2.org - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: coding conventions (was: Re: [Axis2] Issues with the current code base)
HI all, We already have a document which talks about the code quality [1]. We can add whatever more details in to there.. We also need a BIG BOLD lettered link to this page.. We have earlier agreed on using java coding conventions.. But IMHO It's always implied that a java developer will adhere to those. Also coding conventions is important thing we *MUST* be looking at when we are voting somebody as a committer. That's why we need to look at a considerable number of commits before proposing somebody as a committer.. (Of course this does not apply to doc committers).. We can add these to the http://ws.apache.org/axis2/guidelines.html document as discussed in this thread [2]. ~Thilina [1] http://wiki.apache.org/ws/FrontPage/Axis2/CodeQuality [2] http://marc.theaimsgroup.com/?l=axis-devm=112115920228742w=2 On 1/30/07, Sanjiva Weerawarana [EMAIL PROTECTED] wrote: On Mon, 2007-01-29 at 13:37 -0800, Bill Nagy wrote: I was unable to find them on the wiki (I looked at both the current as well as the old root pages.) The JAX-WS proposal only speaks in the abstract about code organization -- it says nothing about the formatting of Java source files. Certainly you can't expect people to adhere to coding guidelines that only appear in email messages from almost 2 years ago or even know that they exist in the first place. Absolutely; fair enough. The basic rule was that we're following the Sun Java coding conventions (http://java.sun.com/docs/codeconv/) except for the number of columns used. I argued hard for 80 col but was voted down .. we settled at 100, which was the middle between 80 and 120 ;(. Does anyone need more details? If not please conform. (Old timers: if you remember the thread and can give a ref that maybe useful.) If someone volunteers to put this in the Wiki in big bold letters that'll be great. Thanks Bill for pushing this to closure! Sanjiva. p.s.: Don't take this to mean that only new folks are guilty of violating the conventions .. everyone is :(. But we need to try to do better .. and its easier to get folks to get in line as they start rather than later. Still, if you find offenders please jump on them! (In a nice warm, fuzzy kinda way ;-)) -- Sanjiva Weerawarana, Ph.D. Founder Director; Lanka Software Foundation; http://www.opensource.lk/ Founder, Chairman CEO; WSO2, Inc.; http://www.wso2.com/ Director; Open Source Initiative; http://www.opensource.org/ Member; Apache Software Foundation; http://www.apache.org/ Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Thilina Gunarathne WSO2, Inc.; http://www.wso2.com/ Home page: http://webservices.apache.org/~thilina/ Blog: http://thilinag.blogspot.com/ - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Axis2] Issues with the current code base
On 1/29/07, Deepal Jayasinghe [EMAIL PROTECTED] wrote: Hi all; I just went though the current code base and realized that MessageContext code has been changed a lot . I found few issues with the code base and hope we need to fix them. So I thought of sending this mail for everyone's consideration. Well someone please explain to me whose going to need MessageContext serialization , Chamikara : Do you want that for Sandesha ? Currently no. But Matt had mentioned that he is working on something that uses this. Matt ? Ruchith : Do you want that for Security ? If none of you want this , who else need this ? I am asking this question b'coz introduction of MC serialization we have the following for each req. - When ever Axis2 received a message it calls activateMessageContext(msgContext); - And what that does is , it calls mc.activate(engineContext); method Unfortunately that method is TOO long and was very difficult to understand :). Ann can you simplify the method (that will be very helpful) . In the meantime code convention in MC has changed a lot and need to have very consistent code convention, please do not differ form that. Among the all , the most worst thing I saw in the code is following kind of things, I strongly believe we should not have this kind of code in the code base, If you found such kind of code please point out them then and there. - String tmpClassNameStr = null; (is this the way we initialize to NULL ) - String tmpHasList = no list - Unnecessary casting - A number of unused variables - Variable declarations here and there (as an example private static final String - selfManagedDataDelimiter = *;) In addition to that someone has added code to change OutInAxisOperation name in the client side temporally, can I know why ? So please comment your ideas on this. Thanks Deepal - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
