[FlexJS] trace statements in SDK code

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[FlexJS] trace statements in SDK code

Justin Mclean
Administrator
Hi,

I’ve noticed in a few places there are some unnecessary trace statements in the SDK. Like for instance here [1].

I’m thinking we probably shouldn’t have trace statements in production code. Sonar cube flags it as an issue. [2] Anyone think otherwise?

On the JS side these are only omitted when goog.DEBUG is defined but still be a little expensive at it calls slice before doing that.

Thanks,
Justin

1. https://github.com/apache/flex-asjs/blob/fd709d137e5f740fd3a7f7dfbcde0e898e25f103/frameworks/projects/DragDrop/src/main/flex/org/apache/flex/html/beads/controllers/DragMouseController.as
2. https://builds.apache.org/analysis/component_issues/index?id=org.apache.flex.flexjs.framework%3Aflexjs-framework-parent#resolved=false|severities=CRITICAL|rules=flex%3AS1951
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [FlexJS] trace statements in SDK code

piotrz
Hi Justin,

+1
Agree with you, we should get rid of them.

Thanks,
Piotr
Apache Flex PMC
piotrzarzycki21@gmail.com
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [FlexJS] trace statements in SDK code

Alex Harui-2
In reply to this post by Justin Mclean
Interesting topic.  Is this something that folks think is important for
this release?

I would not want to have a "no trace statements" policy.  IMO trace
statements are a useful tool when used appropriately.  As long as the tool
chain can remove trace statements in production code, that should be all
we need.  But I think it is too late to be dealing with this for this
release.

I use trace statements when I'm not sure about some code condition and
when there is some tricky thing that can be debugged by examining a log of
output.  Throwing an exception stops execution and it cannot be easily
restarted.  A trace statement says "hey, I think the code didn't expect
this condition" but allows execution to continue.

The only "rule" we had in regular Flex was that trace statements should
pollute your output with tons of stuff.  The checkintests checked for
unexpected trace output in the main code paths of the components.  Many
trace statements were commented out or behind logic that kept them off
until you needed them.

My 2 cents,
-Alex



On 5/23/17, 1:46 AM, "Justin Mclean" <[hidden email]> wrote:

>Hi,
>
>I’ve noticed in a few places there are some unnecessary trace statements
>in the SDK. Like for instance here [1].
>
>I’m thinking we probably shouldn’t have trace statements in production
>code. Sonar cube flags it as an issue. [2] Anyone think otherwise?
>
>On the JS side these are only omitted when goog.DEBUG is defined but
>still be a little expensive at it calls slice before doing that.
>
>Thanks,
>Justin
>
>1.
>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
>m%2Fapache%2Fflex-asjs%2Fblob%2Ffd709d137e5f740fd3a7f7dfbcde0e898e25f103%2
>Fframeworks%2Fprojects%2FDragDrop%2Fsrc%2Fmain%2Fflex%2Forg%2Fapache%2Ffle
>x%2Fhtml%2Fbeads%2Fcontrollers%2FDragMouseController.as&data=02%7C01%7C%7C
>2d1d49dd9de4462348e108d4a1b847e6%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C
>0%7C636311260235128408&sdata=mpAT3eCx2Cz1IUSRA8M07s2oW4xo%2BpqIm0Jliu%2BkQ
>24%3D&reserved=0
>2.
>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbuilds.ap
>ache.org%2Fanalysis%2Fcomponent_issues%2Findex%3Fid%3Dorg.apache.flex.flex
>js.framework%253Aflexjs-framework-parent%23resolved%3Dfalse%7Cseverities%3
>DCRITICAL%7Crules%3Dflex%253AS1951&data=02%7C01%7C%7C2d1d49dd9de4462348e10
>8d4a1b847e6%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C63631126023512840
>8&sdata=agE57nLohxBpNqBMz9pVKKNTOeB6qRxWnu%2FMBM4bpaw%3D&reserved=0

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [FlexJS] trace statements in SDK code

Christofer Dutz
As far as I understood it, right now debug is the only option as all working on larger applications can’t use the release build.
I think trace statements should eventually be wrapped by a “DEBUG” define to allow including them and have them excluded by default.
We are talking about efficiency of produced code … for me the traces are an equally anti-pattern as are the System.out.printline statelents in the Compiler.

I wouldn’t call it a blocker for the next release, but definitely one for 1.0.0

Chris

Am 23.05.17, 16:55 schrieb "Alex Harui" <[hidden email]>:

    Interesting topic.  Is this something that folks think is important for
    this release?
   
    I would not want to have a "no trace statements" policy.  IMO trace
    statements are a useful tool when used appropriately.  As long as the tool
    chain can remove trace statements in production code, that should be all
    we need.  But I think it is too late to be dealing with this for this
    release.
   
    I use trace statements when I'm not sure about some code condition and
    when there is some tricky thing that can be debugged by examining a log of
    output.  Throwing an exception stops execution and it cannot be easily
    restarted.  A trace statement says "hey, I think the code didn't expect
    this condition" but allows execution to continue.
   
    The only "rule" we had in regular Flex was that trace statements should
    pollute your output with tons of stuff.  The checkintests checked for
    unexpected trace output in the main code paths of the components.  Many
    trace statements were commented out or behind logic that kept them off
    until you needed them.
   
    My 2 cents,
    -Alex
   
   
   
    On 5/23/17, 1:46 AM, "Justin Mclean" <[hidden email]> wrote:
   
    >Hi,
    >
    >I’ve noticed in a few places there are some unnecessary trace statements
    >in the SDK. Like for instance here [1].
    >
    >I’m thinking we probably shouldn’t have trace statements in production
    >code. Sonar cube flags it as an issue. [2] Anyone think otherwise?
    >
    >On the JS side these are only omitted when goog.DEBUG is defined but
    >still be a little expensive at it calls slice before doing that.
    >
    >Thanks,
    >Justin
    >
    >1.
    >https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
    >m%2Fapache%2Fflex-asjs%2Fblob%2Ffd709d137e5f740fd3a7f7dfbcde0e898e25f103%2
    >Fframeworks%2Fprojects%2FDragDrop%2Fsrc%2Fmain%2Fflex%2Forg%2Fapache%2Ffle
    >x%2Fhtml%2Fbeads%2Fcontrollers%2FDragMouseController.as&data=02%7C01%7C%7C
    >2d1d49dd9de4462348e108d4a1b847e6%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C
    >0%7C636311260235128408&sdata=mpAT3eCx2Cz1IUSRA8M07s2oW4xo%2BpqIm0Jliu%2BkQ
    >24%3D&reserved=0
    >2.
    >https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbuilds.ap
    >ache.org%2Fanalysis%2Fcomponent_issues%2Findex%3Fid%3Dorg.apache.flex.flex
    >js.framework%253Aflexjs-framework-parent%23resolved%3Dfalse%7Cseverities%3
    >DCRITICAL%7Crules%3Dflex%253AS1951&data=02%7C01%7C%7C2d1d49dd9de4462348e10
    >8d4a1b847e6%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C63631126023512840
    >8&sdata=agE57nLohxBpNqBMz9pVKKNTOeB6qRxWnu%2FMBM4bpaw%3D&reserved=0
   
   

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [FlexJS] trace statements in SDK code

Alex Harui-2
It might be as simple as not setting @export on Language.utils.trace.  The
optimizer might then figure it out without having to add DEBUG defines.
Maybe something to investigate and discuss further after we get this
release out?

-Alex

On 5/23/17, 8:03 AM, "Christofer Dutz" <[hidden email]> wrote:

>As far as I understood it, right now debug is the only option as all
>working on larger applications can’t use the release build.
>I think trace statements should eventually be wrapped by a “DEBUG” define
>to allow including them and have them excluded by default.
>We are talking about efficiency of produced code … for me the traces are
>an equally anti-pattern as are the System.out.printline statelents in the
>Compiler.
>
>I wouldn’t call it a blocker for the next release, but definitely one for
>1.0.0
>
>Chris
>
>Am 23.05.17, 16:55 schrieb "Alex Harui" <[hidden email]>:
>
>    Interesting topic.  Is this something that folks think is important
>for
>    this release?
>    
>    I would not want to have a "no trace statements" policy.  IMO trace
>    statements are a useful tool when used appropriately.  As long as the
>tool
>    chain can remove trace statements in production code, that should be
>all
>    we need.  But I think it is too late to be dealing with this for this
>    release.
>    
>    I use trace statements when I'm not sure about some code condition and
>    when there is some tricky thing that can be debugged by examining a
>log of
>    output.  Throwing an exception stops execution and it cannot be easily
>    restarted.  A trace statement says "hey, I think the code didn't
>expect
>    this condition" but allows execution to continue.
>    
>    The only "rule" we had in regular Flex was that trace statements
>should
>    pollute your output with tons of stuff.  The checkintests checked for
>    unexpected trace output in the main code paths of the components.
>Many
>    trace statements were commented out or behind logic that kept them off
>    until you needed them.
>    
>    My 2 cents,
>    -Alex
>    
>    
>    
>    On 5/23/17, 1:46 AM, "Justin Mclean" <[hidden email]> wrote:
>    
>    >Hi,
>    >
>    >I’ve noticed in a few places there are some unnecessary trace
>statements
>    >in the SDK. Like for instance here [1].
>    >
>    >I’m thinking we probably shouldn’t have trace statements in
>production
>    >code. Sonar cube flags it as an issue. [2] Anyone think otherwise?
>    >
>    >On the JS side these are only omitted when goog.DEBUG is defined but
>    >still be a little expensive at it calls slice before doing that.
>    >
>    >Thanks,
>    >Justin
>    >
>    >1.
>    
>>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
>>o
>    
>>m%2Fapache%2Fflex-asjs%2Fblob%2Ffd709d137e5f740fd3a7f7dfbcde0e898e25f103%
>>2
>    
>>Fframeworks%2Fprojects%2FDragDrop%2Fsrc%2Fmain%2Fflex%2Forg%2Fapache%2Ffl
>>e
>    
>>x%2Fhtml%2Fbeads%2Fcontrollers%2FDragMouseController.as&data=02%7C01%7C%7
>>C
>    
>>2d1d49dd9de4462348e108d4a1b847e6%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7
>>C
>    
>>0%7C636311260235128408&sdata=mpAT3eCx2Cz1IUSRA8M07s2oW4xo%2BpqIm0Jliu%2Bk
>>Q
>    >24%3D&reserved=0
>    >2.
>    
>>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbuilds.a
>>p
>    
>>ache.org%2Fanalysis%2Fcomponent_issues%2Findex%3Fid%3Dorg.apache.flex.fle
>>x
>    
>>js.framework%253Aflexjs-framework-parent%23resolved%3Dfalse%7Cseverities%
>>3
>    
>>DCRITICAL%7Crules%3Dflex%253AS1951&data=02%7C01%7C%7C2d1d49dd9de4462348e1
>>0
>    
>>8d4a1b847e6%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C6363112602351284
>>0
>    >8&sdata=agE57nLohxBpNqBMz9pVKKNTOeB6qRxWnu%2FMBM4bpaw%3D&reserved=0
>    
>    
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [FlexJS] trace statements in SDK code

Christofer Dutz
+1 for that

Am 23.05.17, 18:36 schrieb "Alex Harui" <[hidden email]>:

    It might be as simple as not setting @export on Language.utils.trace.  The
    optimizer might then figure it out without having to add DEBUG defines.
    Maybe something to investigate and discuss further after we get this
    release out?
   
    -Alex
   
    On 5/23/17, 8:03 AM, "Christofer Dutz" <[hidden email]> wrote:
   
    >As far as I understood it, right now debug is the only option as all
    >working on larger applications can’t use the release build.
    >I think trace statements should eventually be wrapped by a “DEBUG” define
    >to allow including them and have them excluded by default.
    >We are talking about efficiency of produced code … for me the traces are
    >an equally anti-pattern as are the System.out.printline statelents in the
    >Compiler.
    >
    >I wouldn’t call it a blocker for the next release, but definitely one for
    >1.0.0
    >
    >Chris
    >
    >Am 23.05.17, 16:55 schrieb "Alex Harui" <[hidden email]>:
    >
    >    Interesting topic.  Is this something that folks think is important
    >for
    >    this release?
    >    
    >    I would not want to have a "no trace statements" policy.  IMO trace
    >    statements are a useful tool when used appropriately.  As long as the
    >tool
    >    chain can remove trace statements in production code, that should be
    >all
    >    we need.  But I think it is too late to be dealing with this for this
    >    release.
    >    
    >    I use trace statements when I'm not sure about some code condition and
    >    when there is some tricky thing that can be debugged by examining a
    >log of
    >    output.  Throwing an exception stops execution and it cannot be easily
    >    restarted.  A trace statement says "hey, I think the code didn't
    >expect
    >    this condition" but allows execution to continue.
    >    
    >    The only "rule" we had in regular Flex was that trace statements
    >should
    >    pollute your output with tons of stuff.  The checkintests checked for
    >    unexpected trace output in the main code paths of the components.
    >Many
    >    trace statements were commented out or behind logic that kept them off
    >    until you needed them.
    >    
    >    My 2 cents,
    >    -Alex
    >    
    >    
    >    
    >    On 5/23/17, 1:46 AM, "Justin Mclean" <[hidden email]> wrote:
    >    
    >    >Hi,
    >    >
    >    >I’ve noticed in a few places there are some unnecessary trace
    >statements
    >    >in the SDK. Like for instance here [1].
    >    >
    >    >I’m thinking we probably shouldn’t have trace statements in
    >production
    >    >code. Sonar cube flags it as an issue. [2] Anyone think otherwise?
    >    >
    >    >On the JS side these are only omitted when goog.DEBUG is defined but
    >    >still be a little expensive at it calls slice before doing that.
    >    >
    >    >Thanks,
    >    >Justin
    >    >
    >    >1.
    >    
    >>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
    >>o
    >    
    >>m%2Fapache%2Fflex-asjs%2Fblob%2Ffd709d137e5f740fd3a7f7dfbcde0e898e25f103%
    >>2
    >    
    >>Fframeworks%2Fprojects%2FDragDrop%2Fsrc%2Fmain%2Fflex%2Forg%2Fapache%2Ffl
    >>e
    >    
    >>x%2Fhtml%2Fbeads%2Fcontrollers%2FDragMouseController.as&data=02%7C01%7C%7
    >>C
    >    
    >>2d1d49dd9de4462348e108d4a1b847e6%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7
    >>C
    >    
    >>0%7C636311260235128408&sdata=mpAT3eCx2Cz1IUSRA8M07s2oW4xo%2BpqIm0Jliu%2Bk
    >>Q
    >    >24%3D&reserved=0
    >    >2.
    >    
    >>https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbuilds.a
    >>p
    >    
    >>ache.org%2Fanalysis%2Fcomponent_issues%2Findex%3Fid%3Dorg.apache.flex.fle
    >>x
    >    
    >>js.framework%253Aflexjs-framework-parent%23resolved%3Dfalse%7Cseverities%
    >>3
    >    
    >>DCRITICAL%7Crules%3Dflex%253AS1951&data=02%7C01%7C%7C2d1d49dd9de4462348e1
    >>0
    >    
    >>8d4a1b847e6%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C6363112602351284
    >>0
    >    >8&sdata=agE57nLohxBpNqBMz9pVKKNTOeB6qRxWnu%2FMBM4bpaw%3D&reserved=0
    >    
    >    
    >
   
   

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [FlexJS] trace statements in SDK code

Justin Mclean
Administrator
In reply to this post by Alex Harui-2
Hi,

> Interesting topic.  Is this something that folks think is important for
> this release?

Again IMO no need to hold up this release.

> I would not want to have a "no trace statements" policy.  IMO trace
> statements are a useful tool when used appropriately.  As long as the tool
> chain can remove trace statements in production code

Currently while trace statements are omitted in production, there still a cost of a function call, split and static lookup (on the JS side) - split may be expensive but I’ve not profiled it. This would add if we had lots of trace statements in the SDK.

The way this is usually done is with logging system rather than single trace statements. You get a few extra features that way and a little bit more control, that may however come at a runtime cost / size cost as well.

> The only "rule" we had in regular Flex was that trace statements should
> pollute your output with tons of stuff.

The Mouse drag trace statements currently in the SDK seem to be a candidate for removal at least IMO.

Thanks,
Justin
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [FlexJS] trace statements in SDK code

Alex Harui-2
I don't think you get my point, but let's discuss further after the
release.

Thanks,
-Alex

On 5/23/17, 3:03 PM, "Justin Mclean" <[hidden email]> wrote:

>Hi,
>
>> Interesting topic.  Is this something that folks think is important for
>> this release?
>
>Again IMO no need to hold up this release.
>
>> I would not want to have a "no trace statements" policy.  IMO trace
>> statements are a useful tool when used appropriately.  As long as the
>>tool
>> chain can remove trace statements in production code
>
>Currently while trace statements are omitted in production, there still a
>cost of a function call, split and static lookup (on the JS side) - split
>may be expensive but I’ve not profiled it. This would add if we had lots
>of trace statements in the SDK.
>
>The way this is usually done is with logging system rather than single
>trace statements. You get a few extra features that way and a little bit
>more control, that may however come at a runtime cost / size cost as well.
>
>> The only "rule" we had in regular Flex was that trace statements should
>> pollute your output with tons of stuff.
>
>The Mouse drag trace statements currently in the SDK seem to be a
>candidate for removal at least IMO.
>
>Thanks,
>Justin

Loading...