Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

piotrz
Hi Justin

What will happen if some of those values will be null ? For example
styleable.style ?

Piotr

2017-06-01 7:58 GMT+02:00 <[hidden email]>:

> make getValue a lot faster - 30ms down to 5ms in a complex app for JS in
> Chrome
>
>
> Project: http://git-wip-us.apache.org/repos/asf/flex-asjs/repo
> Commit: http://git-wip-us.apache.org/repos/asf/flex-asjs/commit/5e0d3305
> Tree: http://git-wip-us.apache.org/repos/asf/flex-asjs/tree/5e0d3305
> Diff: http://git-wip-us.apache.org/repos/asf/flex-asjs/diff/5e0d3305
>
> Branch: refs/heads/develop
> Commit: 5e0d33052ad6105cd92b18e1ce800674ce1ccec9
> Parents: bdbffb8
> Author: Justin Mclean <[hidden email]>
> Authored: Thu Jun 1 15:58:01 2017 +1000
> Committer: Justin Mclean <[hidden email]>
> Committed: Thu Jun 1 15:58:01 2017 +1000
>
> ----------------------------------------------------------------------
>  .../org/apache/flex/core/SimpleCSSValuesImpl.as | 43 ++++++++------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/flex-asjs/blob/
> 5e0d3305/frameworks/projects/Core/src/main/flex/org/apache/
> flex/core/SimpleCSSValuesImpl.as
> ----------------------------------------------------------------------
> diff --git a/frameworks/projects/Core/src/main/flex/org/apache/flex/core/SimpleCSSValuesImpl.as
> b/frameworks/projects/Core/src/main/flex/org/apache/flex/
> core/SimpleCSSValuesImpl.as
> index 6d4adec..a4e4e1d 100644
> --- a/frameworks/projects/Core/src/main/flex/org/apache/flex/
> core/SimpleCSSValuesImpl.as
> +++ b/frameworks/projects/Core/src/main/flex/org/apache/flex/
> core/SimpleCSSValuesImpl.as
> @@ -99,8 +99,8 @@ package org.apache.flex.core
>              while (true)
>              {
>                  var ffName:String = "factoryFunctions" + i.toString();
> -                var ff:Object = c[ffName];
> -                if (ff === null)
> +                var ff:* = c[ffName];
> +                if (ff === undefined)
>                      break;
>                  generateCSSStyleDeclarations(c[ffName], c["data" +
> i.toString()]);
>                  if (hasEventListener("init"))
> @@ -164,9 +164,7 @@ package org.apache.flex.core
>          COMPILE::SWF
>          public function generateCSSStyleDeclarations(factoryFunctions:Object,
> arr:Array):void
>          {
> -                       if (factoryFunctions === null)
> -                               return;
> -                       if (arr === null)
> +                       if (factoryFunctions === null || arr === null)
>                                 return;
>
>              var declarationName:String = "";
> @@ -331,30 +329,25 @@ package org.apache.flex.core
>              }
>
>              var value:*;
> -                       var o:Object;
> +                       var o:*;
>                         var className:String;
>                         var selectorName:String;
>
>                         if (thisObject is IStyleableObject)
>                         {
>                  var styleable:IStyleableObject =
> IStyleableObject(thisObject);
> -                if (styleable.style !== null)
> +                if (styleable.style !== undefined)
>                  {
> -                    try {
> -                        value = styleable.style[valueName];
> -                    }
> -                    catch (e:Error) {
> -                        value = undefined;
> -                    }
> +                    value = styleable.style[valueName];
>                      if (value === INHERIT)
>                          return getInheritingValue(thisObject, valueName,
> state, attrs);
>                      if (value !== undefined)
>                          return value;
>                  }
> -                if (styleable.id !== null)
> +                if (styleable.id !== undefined)
>                  {
>                      o = values["#" + styleable.id];
> -                    if (o)
> +                    if (o !== undefined)
>                      {
>                          value = o[valueName];
>                          if (value === INHERIT)
> @@ -364,16 +357,16 @@ package org.apache.flex.core
>                      }
>                  }
>                                 var classNames:String =
> styleable.className;
> -                if (classNames)
> +                if (classNames !== undefined)
>                  {
>                      var classNameList:Array = classNames.split(" ");
>                      for each (className in classNameList)
>                      {
> -                        if (state)
> +                        if (state !== null)
>                          {
>                              selectorName = className + ":" + state;
>                              o = values["." + selectorName];
> -                            if (o)
> +                            if (o !== undefined)
>                              {
>                                  value = o[valueName];
>                                  if (value === INHERIT)
> @@ -384,7 +377,7 @@ package org.apache.flex.core
>                          }
>
>                          o = values["." + className];
> -                        if (o)
> +                        if (o !== undefined)
>                          {
>                              value = o[valueName];
>                              if (value === INHERIT)
> @@ -407,11 +400,11 @@ package org.apache.flex.core
>              var thisInstance:Object = thisObject;
>                         while (className !== "Object")
>                         {
> -                               if (state)
> +                               if (state !== null)
>                                 {
>                                         selectorName = className + ":" +
> state;
>                                         o = values[selectorName];
> -                                       if (o)
> +                                       if (o !== undefined)
>                                         {
>                                                 value = o[valueName];
>                          if (value === INHERIT)
> @@ -422,7 +415,7 @@ package org.apache.flex.core
>                                 }
>
>                     o = values[className];
> -                   if (o)
> +                   if (o !== undefined)
>                     {
>                         value = o[valueName];
>                      if (value === INHERIT)
> @@ -446,7 +439,7 @@ package org.apache.flex.core
>                  }
>                         }
>
> -            if (inheritingStyles[valueName] != null &&
> +            if (inheritingStyles[valueName] !== undefined &&
>                  thisObject is IChild)
>              {
>                  var parentObject:Object = IChild(thisObject).parent;
> @@ -455,14 +448,14 @@ package org.apache.flex.core
>              }
>
>              o = values["global"];
> -            if (o)
> +            if (o !== undefined)
>              {
>                         value = o[valueName];
>                         if (value !== undefined)
>                                 return value;
>              }
>                         o = values["*"];
> -                       if(o)
> +                       if (o !== undefined)
>                         {
>                                 return o[valueName];
>                         }
>
>
Apache Flex PMC
piotrzarzycki21@gmail.com
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
Hi,

> What will happen if some of those values will be null ? For example styleable.style ?

Styleable could be I guess but there wasn’t a null check for it before.

Styable.style is unlikely to be null. All test still pass and I’ve tested this with several applications and it never has a null value and there no code in the framework that sets it to null that I could see. The code is significantly faster.

In the other case you are looking a property that's likely to be a string /number so it will exist or not exists, comparing via == or != or if(o) is dangerous as it matches or doesn’t match all sort of things you may not expect.

But as AS and JS behaves a little differently here it would be good to have a discussion on how we should treat null/undefined.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

piotrz
Hi Justin,

I just tried develop and build framework.

1) I see following warnings [1].
2) I build "DataBindingExampleWithFlexLayout" and run swf version and I see Null Pointer Exception in SimpleCSSValuesImpl.



[1] https://paste.apache.org/oexm

Could you please take a look into that ? It seems to be related to your changes from this commit.

Thanks,
Piotr

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
HI,

> 1) I see following warnings [1].

The warnings are fine or rather the compiler shouldn’t be warning there - this has been discussed before on the list.

> 2) I build "DataBindingExampleWithFlexLayout" and run swf version and I see
> Null Pointer Exception in SimpleCSSValuesImpl.

That I’ll take a look at.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

piotrz
This post was updated on .
Justin,

If it was discussed and the other were agree for the warnings can you raise a jira to remove them later on ?

Additional things I was speaking a bit with colleagues from JS team in my company and asking them how to check "undefined" things. They were saying that they are checking such things in that way:

if(typeof a !== 'undefined') {
}

And of course if we consider null:

if(typeof a !== 'undefined' && null !== a) {
}

Thoughts ?

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
Hi,

> if(typeof a !== 'undefined') {
> }
>
> And of course if we consider null:
>
> if(typeof a !== 'undefined' && null !== a) {
> }
>
> Thoughts ?

It’s really going to depend on the context / code.

In some case it will be obvious with one to check. In others we may need to change the code to make it clear which values are valid.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

piotrz
I'm not sure whether I understand your answer.

I don't actually talking about checking between null or undefined - cause it's obvious it is depends on scenario.

I'm talking about a way in which we are doing it - What do you thing to use this "typeof a". ?

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
In reply to this post by Justin Mclean
Hi,

>> 2) I build "DataBindingExampleWithFlexLayout" and run swf version and I see
>> Null Pointer Exception in SimpleCSSValuesImpl.
>
> That I’ll take a look at.

It’s running file here for me. What browser / what was the error you were getting?

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

piotrz
I didn't run it in the browser. I run swf in Flash Player projector [1].

[1] http://www.adobe.com/support/flashplayer/debug_downloads.html
Apache Flex PMC
piotrzarzycki21@gmail.com
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
In reply to this post by piotrz
Hi,

> if(typeof a !== 'undefined') {
> }

Are we really concerned that undefined may of been redefined? (Which would probably cause a whole lot of issues.)

Seems it may be a little expensive to me compared to a straight === undefined.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
In reply to this post by piotrz
HI,

> I didn't run it in the browser. I run swf in Flash Player projector [1].

How are you compiling that? Looks like "mvn compile” only produces the JS version.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

piotrz
Justin,

I just pushed fix and it should build both version.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
Hi,

OK the issue is that we getting different results for AS and JS.

For code like this:
var a:Object = {};
var propA:Object = a.prop;
var propB:* = a.prop;
if (propA === undefined) {
        trace("A undefined");
}
if (propA === null) {
        trace("A null");
}
if (propB === undefined) {
        trace("B undefined");
}
if (propB === null) {
        trace("B null");
}

On AS we get:
A null
B undefined

On JS we get:
A undefined
B undefined

Which is obvious when you look at the generated code:
 var /** @type {Object} */ a = {};
 var /** @type {Object} */ propA = a.prop;
 var /** @type {*} */ propB = a.prop;

So I’m guessing the compiler should be doing something different with this line?
 var /** @type {Object} */ propA = a.prop;

So it returns null rather than undefined?

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Josh Tynjala
Another place where things differ is member variables:

public var a:String;

On the SWF side, this variable defaults to null. On the JS side, it
defaults to undefined right now.

In the case of a member variable, we could set it to null by default on the
JS side to make it more consistent.

For your case, it's a little more tricky, though. Maybe something like
this, if the type on the left side is anything except *? (Numbers and
boolean would need special behavior too, though)

var propA:Object = a.prop || null;

That could have memory or performance implications. Sometimes, trying to
achieve perfect behavior parity can be too expensive. Needs benchmarking
and careful analysis to see if it's worth it, I guess!

- Josh


On Jun 2, 2017 5:20 PM, "Justin Mclean" <[hidden email]> wrote:

Hi,

OK the issue is that we getting different results for AS and JS.

For code like this:
var a:Object = {};
var propA:Object = a.prop;
var propB:* = a.prop;
if (propA === undefined) {
        trace("A undefined");
}
if (propA === null) {
        trace("A null");
}
if (propB === undefined) {
        trace("B undefined");
}
if (propB === null) {
        trace("B null");
}

On AS we get:
A null
B undefined

On JS we get:
A undefined
B undefined

Which is obvious when you look at the generated code:
 var /** @type {Object} */ a = {};
 var /** @type {Object} */ propA = a.prop;
 var /** @type {*} */ propB = a.prop;

So I’m guessing the compiler should be doing something different with this
line?
 var /** @type {Object} */ propA = a.prop;

So it returns null rather than undefined?

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

piotrz
Hi Justin,

I saw your commit. Why in that case you cannot have?

if(styleable.style !== 'undefined' && styleable.style != null)

You will have both cases handled.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
Hi,

> if(styleable.style !== 'undefined' && styleable.style != null)
>
> You will have both cases handled.

(styleable.style != null) will cover both cases as well - and probably a few you may not want :-)

Until we sort out the way to do this I've just put in something that worked on both platforms.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

piotrz
Ahh!! You are right. Stupid. Sorry about that.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
Hi,

> Ahh!! You are right. Stupid. Sorry about that.

No reason to be :-)  Knowing what evacuates to true with == and false with != is hard to remember and get right.

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

Re: [7/7] git commit: [flex-asjs] [refs/heads/develop] - make getValue a lot faster - 30ms down to 5ms in a complex app for JS in Chrome

Justin Mclean
Administrator
Hi,

> No reason to be :-)  Knowing what evacuates to true with == and false with != is hard to remember and get right.

Evaluates was meant obviously. Autocorrect can sometime give amusing results.

thanks,
Justin
Loading...