New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

angular.copy Very poor performance (large objects) #11099

Open
Tomino2112 opened this Issue Feb 18, 2015 · 40 comments

Comments

Projects
None yet
@Tomino2112

Tomino2112 commented Feb 18, 2015

Our application loads resources from API and renders them in a table. API request takes ~200ms, rendering table ~140ms but when you actually looking at the page it takes about 6s to display it.

I have drilled through the code and realized this was caused by using angular.copy . I need to keep track of the old data so I copied the whole object to backup object via angular.copy() function. The copying itself took over 2s (~2500ms).

I have searched for better ways, to copy objects and stumbled upon this code:

cloneObject: function(o){
            const gdcc = "__getDeepCircularCopy__";
            if (o !== Object(o)) {
                return o; // primitive value
            }

            var set = gdcc in o,
                cache = o[gdcc],
                result;
            if (set && typeof cache == "function") {
                return cache();
            }
            // else
            o[gdcc] = function() { return result; }; // overwrite
            if (o instanceof Array) {
                result = [];
                for (var i=0; i<o.length; i++) {
                    result[i] = this.cloneObject(o[i]);
                }
            } else {
                result = {};
                for (var prop in o)
                    if (prop != gdcc)
                        result[prop] = this.cloneObject(o[prop]);
                    else if (set)
                        result[prop] = this.cloneObject(cache);
            }
            if (set) {
                o[gdcc] = cache; // reset
            } else {
                delete o[gdcc]; // unset again
            }
            return result;
        },

This is my implementation of this: http://stackoverflow.com/a/10729242/820942

Replacing all angular.copy() in my code with this hugely improved the webapp overall. The same example above takes ~41ms instead of ~2s.

I should mention that the data I am mentioning is pretty large (renders into 10 000 cells table)

After very brief look on the angular.copy code I am aware of that there are some added angular goodies ($$hashkey) but still the difference in performance is just too big.

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Feb 21, 2015

Member

@Tomino2112 did you try to put this code and see if it passes all the existing tests? As you've noticed, angular.copy is more of an internal utility and we kind of regret exposing it as a public API... but now the damage is done... So if your version works better for your particular case - by all means use it!

It is kind of hard to say more without an isolated reproduction scenario which we could profile to actually see what is going on. We are missing this as well as other essential info (ex.: browser and its version used for tests).

In short: please provide an isolated reproduce scenario, otherwise it is not very actionable.

Member

pkozlowski-opensource commented Feb 21, 2015

@Tomino2112 did you try to put this code and see if it passes all the existing tests? As you've noticed, angular.copy is more of an internal utility and we kind of regret exposing it as a public API... but now the damage is done... So if your version works better for your particular case - by all means use it!

It is kind of hard to say more without an isolated reproduction scenario which we could profile to actually see what is going on. We are missing this as well as other essential info (ex.: browser and its version used for tests).

In short: please provide an isolated reproduce scenario, otherwise it is not very actionable.

@vitaly-t

This comment has been minimized.

Show comment
Hide comment
@vitaly-t

vitaly-t Feb 23, 2015

@Tomino2112, it may be interesting to see if your approach can be somehow used within angular for speed optimization, but in the meantime, it is never a good approach trying to display 10,000 cells on a page at once through a single request. It is a very poor approach from the architecture point of view.

@pkozlowski-opensource, I find his research credible enough to suggest that Angular's implementation for deep copy is to be reviewed, based on implementation by @Tomino2112. The performance difference seems to be huge.

vitaly-t commented Feb 23, 2015

@Tomino2112, it may be interesting to see if your approach can be somehow used within angular for speed optimization, but in the meantime, it is never a good approach trying to display 10,000 cells on a page at once through a single request. It is a very poor approach from the architecture point of view.

@pkozlowski-opensource, I find his research credible enough to suggest that Angular's implementation for deep copy is to be reviewed, based on implementation by @Tomino2112. The performance difference seems to be huge.

@Tomino2112

This comment has been minimized.

Show comment
Hide comment
@Tomino2112

Tomino2112 Feb 23, 2015

Hi guys,
@pkozlowski-opensource When I have a minute I will create sample fiddle. To answer the other questions - no I didn't try to run tests, I am not replacing the angular.copy function, just have it on the side in my "utils" library. The browsers tested were FF, IE and Chrome. Obviously chrome had best results and not so big gap in performance, FF was the one with most obvious performance difference.

@VitalyTomilov Yeah its not great to render that many cells, actually it is edge scenario, usually its around hundereds of cells - still not great though, but thats what you get when you try to render sheets of data online. I have some ideas for rendering on my list, just not enough time to play around with them at the moment.

I will prepare some code example so you cna see it in action (i mean the angular.copy)

Tomino2112 commented Feb 23, 2015

Hi guys,
@pkozlowski-opensource When I have a minute I will create sample fiddle. To answer the other questions - no I didn't try to run tests, I am not replacing the angular.copy function, just have it on the side in my "utils" library. The browsers tested were FF, IE and Chrome. Obviously chrome had best results and not so big gap in performance, FF was the one with most obvious performance difference.

@VitalyTomilov Yeah its not great to render that many cells, actually it is edge scenario, usually its around hundereds of cells - still not great though, but thats what you get when you try to render sheets of data online. I have some ideas for rendering on my list, just not enough time to play around with them at the moment.

I will prepare some code example so you cna see it in action (i mean the angular.copy)

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Mar 2, 2015

Contributor

I think the main thing slowing angular.copy is the use of a stack (arrays) to track the already copied objects. The snippet proposed above modifies the objects being copied which might be faster but will have a lot of issues if you want a general purpose copy method (it won't work at all on immutable objects, what if it crashes in the middle leaving a bunch of objects with modifications, etc.).

#11215 helps a bit mainly by reducing the size of the stack (today sub object/arrays are pushed twice!).

However I have found that replacing the stack with an ES6 Map improves performance significantly. I'm not sure if it's worth shimming Map just for the copy method though. Would Map be useful anywhere else?

Contributor

jbedard commented Mar 2, 2015

I think the main thing slowing angular.copy is the use of a stack (arrays) to track the already copied objects. The snippet proposed above modifies the objects being copied which might be faster but will have a lot of issues if you want a general purpose copy method (it won't work at all on immutable objects, what if it crashes in the middle leaving a bunch of objects with modifications, etc.).

#11215 helps a bit mainly by reducing the size of the stack (today sub object/arrays are pushed twice!).

However I have found that replacing the stack with an ES6 Map improves performance significantly. I'm not sure if it's worth shimming Map just for the copy method though. Would Map be useful anywhere else?

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Mar 2, 2015

Member

I wonder if a shimmed Map would have the same performance benefits as the "native" Map.
Isn't the private HashMap sort of a shim for Map ? Would it help ?

Member

gkalpak commented Mar 2, 2015

I wonder if a shimmed Map would have the same performance benefits as the "native" Map.
Isn't the private HashMap sort of a shim for Map ? Would it help ?

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Mar 2, 2015

Contributor

HashMap modifies the key object so that won't work for this case. A shimmed Map has to use the 2-array/indexOf method so it wouldn't have any performance benefits.

Contributor

jbedard commented Mar 2, 2015

HashMap modifies the key object so that won't work for this case. A shimmed Map has to use the 2-array/indexOf method so it wouldn't have any performance benefits.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Mar 2, 2015

Member

That's what I thought :)

Member

gkalpak commented Mar 2, 2015

That's what I thought :)

@vitaly-t

This comment has been minimized.

Show comment
Hide comment
@vitaly-t

vitaly-t Mar 2, 2015

Maybe it is a bit sinister having just one Copy version for both simple data and immutable objects, so if one wants a simpler, high-performance copy without any advanced provisions, it is not available.

If this is the case, the best approach is to provide two separate Copy implementations either through 2 separate functions or through an extra parameter for Copy.

vitaly-t commented Mar 2, 2015

Maybe it is a bit sinister having just one Copy version for both simple data and immutable objects, so if one wants a simpler, high-performance copy without any advanced provisions, it is not available.

If this is the case, the best approach is to provide two separate Copy implementations either through 2 separate functions or through an extra parameter for Copy.

@sourceRS

This comment has been minimized.

Show comment
Hide comment
@sourceRS

sourceRS Mar 2, 2015

Using something like this:

destination = JSON.parse(JSON.stringify(source));

I copied 10,000 key/value pairs of an object in ~5 milliseconds on my
machine. I know this leaves out a lot of the functionality found in
angular.copy, but perhaps there is some good way of merging this approach
in?

On Mon, Mar 2, 2015 at 10:30 AM, Vitaly Tomilov notifications@github.com
wrote:

Maybe it is a bit sinister having just one Copy version for both simple
data and immutable objects, so if one wants a simpler, high-performance
copy without any advanced provisions, it is not available.

If this is the case, the best approach is to provide two separate Copy
implementations either through 2 separate functions or through an extra
parameter for Copy.


Reply to this email directly or view it on GitHub
#11099 (comment)
.

sourceRS commented Mar 2, 2015

Using something like this:

destination = JSON.parse(JSON.stringify(source));

I copied 10,000 key/value pairs of an object in ~5 milliseconds on my
machine. I know this leaves out a lot of the functionality found in
angular.copy, but perhaps there is some good way of merging this approach
in?

On Mon, Mar 2, 2015 at 10:30 AM, Vitaly Tomilov notifications@github.com
wrote:

Maybe it is a bit sinister having just one Copy version for both simple
data and immutable objects, so if one wants a simpler, high-performance
copy without any advanced provisions, it is not available.

If this is the case, the best approach is to provide two separate Copy
implementations either through 2 separate functions or through an extra
parameter for Copy.


Reply to this email directly or view it on GitHub
#11099 (comment)
.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Mar 3, 2015

Contributor

JSON.parse(JSON.stringify(source)) is often a good method if you know it can handle your data (no circles, no functions, no objects other then string/number/plain-object/null). A standard copy method could never make those assumptions though, and detecting such a thing would require traversing the entire source object which would kill any performance gain. Generally utility methods like this don't belong in angular anyway...

Contributor

jbedard commented Mar 3, 2015

JSON.parse(JSON.stringify(source)) is often a good method if you know it can handle your data (no circles, no functions, no objects other then string/number/plain-object/null). A standard copy method could never make those assumptions though, and detecting such a thing would require traversing the entire source object which would kill any performance gain. Generally utility methods like this don't belong in angular anyway...

@sourceRS

This comment has been minimized.

Show comment
Hide comment
@sourceRS

sourceRS Mar 3, 2015

That's a good point, I didn't realize that code doesn't copy over functions or object methods :/

sourceRS commented Mar 3, 2015

That's a good point, I didn't realize that code doesn't copy over functions or object methods :/

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Mar 3, 2015

Contributor

@gkalpak @jbedard How about using Map in newer browsers and falling back to a shim in old browsers (by my count Internet Explorer 9-10, Safari/iOS 6-7 and Android Browser)

Contributor

realityking commented Mar 3, 2015

@gkalpak @jbedard How about using Map in newer browsers and falling back to a shim in old browsers (by my count Internet Explorer 9-10, Safari/iOS 6-7 and Android Browser)

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Mar 3, 2015

Contributor

@realityking that's what I was proposing, I'm just not sure if creating that shim is worth one use case...

Contributor

jbedard commented Mar 3, 2015

@realityking that's what I was proposing, I'm just not sure if creating that shim is worth one use case...

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Mar 4, 2015

Member

Considering how copy() is used in deep watchers and sometimes in AST.prototype.primary(), it might be worth the trouble.

Two things I would take into account:

  1. How much is the code overhead (in terms of loc).
  2. How much is the performance benefit in "newer" browsers.
Member

gkalpak commented Mar 4, 2015

Considering how copy() is used in deep watchers and sometimes in AST.prototype.primary(), it might be worth the trouble.

Two things I would take into account:

  1. How much is the code overhead (in terms of loc).
  2. How much is the performance benefit in "newer" browsers.
@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Mar 4, 2015

Contributor

When I tried it out (on top of #11215) I think it was ~50 LOC and made all modern browsers ~5x faster with my random over complicated test data. I could open a PR if people think this is worth it?

Contributor

jbedard commented Mar 4, 2015

When I tried it out (on top of #11215) I think it was ~50 LOC and made all modern browsers ~5x faster with my random over complicated test data. I could open a PR if people think this is worth it?

@dpogue

This comment has been minimized.

Show comment
Hide comment
@dpogue

dpogue Mar 8, 2015

👍 for using Map where supported

dpogue commented Mar 8, 2015

👍 for using Map where supported

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Mar 9, 2015

Member

@jbedard what about with not so complex data?

Member

lgalfaso commented Mar 9, 2015

@jbedard what about with not so complex data?

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Mar 10, 2015

Contributor

As the complexity goes down (less objects) using Map has less benefits and is slower with simple data. For example I found copy({a:"b", c: {d: "e"}}) was about 30% slower using Map.

However in those simple data cases it is normally isTypedArray(destination) consuming the most time, not new Map or Map.get/set. So that 30% can easily be made up for with other changes such as jbedard@8a2503b which makes that simple copy 50% faster...

Contributor

jbedard commented Mar 10, 2015

As the complexity goes down (less objects) using Map has less benefits and is slower with simple data. For example I found copy({a:"b", c: {d: "e"}}) was about 30% slower using Map.

However in those simple data cases it is normally isTypedArray(destination) consuming the most time, not new Map or Map.get/set. So that 30% can easily be made up for with other changes such as jbedard@8a2503b which makes that simple copy 50% faster...

@sourceRS

This comment has been minimized.

Show comment
Hide comment
@sourceRS

sourceRS Mar 10, 2015

I don't have time for it right now, but if no one else does it by tomorrow
I can run some tests with varying sizes of each data-type being passed into
copy, both using Map and not using Map. Then we can pinpoint the number of
bytes in the median(s) of the valley(s) in the performance curve and
implement a check where the performance benefits of map outweigh the cons
(as @jbedard pointed out) and then copy can decide whether or not to use it.

What if we did something like the code in the post below?

And figure out where that threshold would be exactly? Or would it even be
worth it to check? Perhaps the performance tradeoffs are negligible? I
don't really know because I haven't run the tests or looked at the data,
but maybe jbedard/angular.js@8a2503b already knows where that would be?

sourceRS commented Mar 10, 2015

I don't have time for it right now, but if no one else does it by tomorrow
I can run some tests with varying sizes of each data-type being passed into
copy, both using Map and not using Map. Then we can pinpoint the number of
bytes in the median(s) of the valley(s) in the performance curve and
implement a check where the performance benefits of map outweigh the cons
(as @jbedard pointed out) and then copy can decide whether or not to use it.

What if we did something like the code in the post below?

And figure out where that threshold would be exactly? Or would it even be
worth it to check? Perhaps the performance tradeoffs are negligible? I
don't really know because I haven't run the tests or looked at the data,
but maybe jbedard/angular.js@8a2503b already knows where that would be?

@sourceRS

This comment has been minimized.

Show comment
Hide comment
@sourceRS

sourceRS Mar 10, 2015

sorry, that code highlighter did not work out lol...

    var byteThreshold = /* Some number of bytes where objects
            of this size or greater are benefited
            by using map, and objects less than
            this byte size take a performance hit
            due to isTypedArray(destination).
            */;

    if (sizeOf(source) > byteThreshold) {
        // use map
    } else {
        // don't use map
    }

sourceRS commented Mar 10, 2015

sorry, that code highlighter did not work out lol...

    var byteThreshold = /* Some number of bytes where objects
            of this size or greater are benefited
            by using map, and objects less than
            this byte size take a performance hit
            due to isTypedArray(destination).
            */;

    if (sizeOf(source) > byteThreshold) {
        // use map
    } else {
        // don't use map
    }
@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Mar 10, 2015

Contributor

It is not worth having 2 implementations...

Contributor

jbedard commented Mar 10, 2015

It is not worth having 2 implementations...

@Narretz Narretz modified the milestones: Ice Box, Purgatory Jun 11, 2015

@Ledzz

This comment has been minimized.

Show comment
Hide comment
@Ledzz

Ledzz Sep 21, 2015

Had an issue: when I try to copy a large object, it copies in ~300ms, but sometimes it can copy for 18-20s. jbedard's method JSON.parse(JSON.stringify(source)) worked good for me.

Ledzz commented Sep 21, 2015

Had an issue: when I try to copy a large object, it copies in ~300ms, but sometimes it can copy for 18-20s. jbedard's method JSON.parse(JSON.stringify(source)) worked good for me.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Oct 27, 2015

Contributor

33c67ce has improved this a bit more often making copy 1.5-3x faster.

Contributor

jbedard commented Oct 27, 2015

33c67ce has improved this a bit more often making copy 1.5-3x faster.

@Tomino2112

This comment has been minimized.

Show comment
Hide comment
@Tomino2112

Tomino2112 Oct 27, 2015

I have tried MANY solutions from all over the internet, but in the end I end-up writing specific function for copying my object. After all the tests and benchmark, the result is, if you have large arrays/objects, its better to write your own copy function specifically for that object with for loops because they are insanely fast compare to whatever other solutions. I still use angular's clone function, but only for small objects

Tomino2112 commented Oct 27, 2015

I have tried MANY solutions from all over the internet, but in the end I end-up writing specific function for copying my object. After all the tests and benchmark, the result is, if you have large arrays/objects, its better to write your own copy function specifically for that object with for loops because they are insanely fast compare to whatever other solutions. I still use angular's clone function, but only for small objects

@frfancha

This comment has been minimized.

Show comment
Hide comment
@frfancha

frfancha Oct 27, 2015

@jbedard Sorry for the naive question, but 33c67ce has a commit date June 9 and doesn't seem included in Angular 1.4.7, could you help me to understand version numbers?
Thanks

frfancha commented Oct 27, 2015

@jbedard Sorry for the naive question, but 33c67ce has a commit date June 9 and doesn't seem included in Angular 1.4.7, could you help me to understand version numbers?
Thanks

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Oct 27, 2015

Contributor

It was just put into master a few days ago so it will be in the next 1.5 release.

Contributor

jbedard commented Oct 27, 2015

It was just put into master a few days ago so it will be in the next 1.5 release.

@frfancha

This comment has been minimized.

Show comment
Hide comment
@frfancha

frfancha Oct 27, 2015

@jbedard Ok thanks for the info. We will try 1.5 then, I'm very curious about the performance difference. I hope there won't be too many breaking changes in 1.5

frfancha commented Oct 27, 2015

@jbedard Ok thanks for the info. We will try 1.5 then, I'm very curious about the performance difference. I hope there won't be too many breaking changes in 1.5

@ksr583 ksr583 referenced this issue Dec 6, 2015

Closed

Angular Issue #49

@stanleyxu2005

This comment has been minimized.

Show comment
Hide comment
@stanleyxu2005

stanleyxu2005 Jan 18, 2016

Maybe there is some performance regression with 1.4.8. After upgrading alone with npm update, I noticed one occurrence with angular.copy() slow down to 2 seconds. I simply replace the function with _.clone() and it shorten to 200ms.

From project practice, I think angular's built-in array/object util functions are not full cover the usage. I have to use lodash (or underscore) anyway. So I think angular should move these helper functions eg. isNumber(), forEach(), copy(), isEqual() into angular-util package, so that the core package size can be even smaller, and we might be able to use lodash instead.

stanleyxu2005 commented Jan 18, 2016

Maybe there is some performance regression with 1.4.8. After upgrading alone with npm update, I noticed one occurrence with angular.copy() slow down to 2 seconds. I simply replace the function with _.clone() and it shorten to 200ms.

From project practice, I think angular's built-in array/object util functions are not full cover the usage. I have to use lodash (or underscore) anyway. So I think angular should move these helper functions eg. isNumber(), forEach(), copy(), isEqual() into angular-util package, so that the core package size can be even smaller, and we might be able to use lodash instead.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Jan 18, 2016

Member

@stanleyxu2005, (as stated before) Angular never intended to provide a replacement for general purpose utility libraries. Its helper functions (copy(), forEach(), isXyz() etc) where implemented for internal use (so they are focused on supporting/performing well only for the intermally needed usecases - they might work well for other usecases, but it's not their purpose).

At that point, it seemed like a good idea to expose the utility functions to the developers (since they were implemented anyway), so they didn't have to import a whole different library for simple usecases. This actually turned out to be a bad idea - among other things, it's much more difficult to make breaking changes to them to better support the evolving needs of the framework.

There's not much we can do now, because removing them would break too many apps. The functions are also used internally, so removing them from the core is not an option (that's their purpose in the first place). So, they'll continue to exist, but users should keep in mind that they might not be the best option for their usecase.

Member

gkalpak commented Jan 18, 2016

@stanleyxu2005, (as stated before) Angular never intended to provide a replacement for general purpose utility libraries. Its helper functions (copy(), forEach(), isXyz() etc) where implemented for internal use (so they are focused on supporting/performing well only for the intermally needed usecases - they might work well for other usecases, but it's not their purpose).

At that point, it seemed like a good idea to expose the utility functions to the developers (since they were implemented anyway), so they didn't have to import a whole different library for simple usecases. This actually turned out to be a bad idea - among other things, it's much more difficult to make breaking changes to them to better support the evolving needs of the framework.

There's not much we can do now, because removing them would break too many apps. The functions are also used internally, so removing them from the core is not an option (that's their purpose in the first place). So, they'll continue to exist, but users should keep in mind that they might not be the best option for their usecase.

@coli

This comment has been minimized.

Show comment
Hide comment
@coli

coli Mar 18, 2016

I just replaced angular.copy with JSON.parse(JSON.stringify()) on a 18meg tree structure. The speed difference was huge...

coli commented Mar 18, 2016

I just replaced angular.copy with JSON.parse(JSON.stringify()) on a 18meg tree structure. The speed difference was huge...

@bcherny

This comment has been minimized.

Show comment
Hide comment
@bcherny

bcherny Jul 7, 2016

Contributor

One more case study....

let as = [...] // array of ~4000 objects

console.time('angular')
angular.copy(as)
console.timeEnd('angular')
// => 71048.443ms

console.time('lodash')
_.cloneDeep(as)
console.timeEnd('lodash')
// => 521.026ms

console.time('json')
JSON.parse(JSON.stringify(as))
console.timeEnd('json')
// => 92.422ms

Please deprecate the public angular.copy API, and add a note on the docs page!

Contributor

bcherny commented Jul 7, 2016

One more case study....

let as = [...] // array of ~4000 objects

console.time('angular')
angular.copy(as)
console.timeEnd('angular')
// => 71048.443ms

console.time('lodash')
_.cloneDeep(as)
console.timeEnd('lodash')
// => 521.026ms

console.time('json')
JSON.parse(JSON.stringify(as))
console.timeEnd('json')
// => 92.422ms

Please deprecate the public angular.copy API, and add a note on the docs page!

@e-cloud

This comment has been minimized.

Show comment
Hide comment
@e-cloud

e-cloud Aug 26, 2016

The team should be aware that source of angular.copy has too much nested functions, that's part of why it's so slow.

e-cloud commented Aug 26, 2016

The team should be aware that source of angular.copy has too much nested functions, that's part of why it's so slow.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Aug 26, 2016

Member

@e-cloud, is there any evidence that inlining the functions would improve performance?

Member

gkalpak commented Aug 26, 2016

@e-cloud, is there any evidence that inlining the functions would improve performance?

@e-cloud

This comment has been minimized.

Show comment
Hide comment
@e-cloud

e-cloud Aug 27, 2016

Nope, what i mean is every time you call angular.copy will create nested functions, which cost more memory and time. Of source, in small scale it doesn't matter.

e-cloud commented Aug 27, 2016

Nope, what i mean is every time you call angular.copy will create nested functions, which cost more memory and time. Of source, in small scale it doesn't matter.

@stanleyxu2005

This comment has been minimized.

Show comment
Hide comment
@stanleyxu2005

stanleyxu2005 Aug 27, 2016

Regarding @coli and @bcherny 's experiment, if the code change is tiny and the performance is remarkable, I'd vote +1 to apply this change. This is a gratis improvement proposal for the angular team, seriously.

angular.copy = function(obj) {
  return JSON.parse(JSON.stringify(obj));
}

@gkalpak Improve performance of frequently use functions is a common sense. I feel a bit confused of using angular functions. If those functions are intended to be used internally, better rename foo to internal.foo or _foo. If we don't have much knowledge of angular's history, we never know these functions are not for public usage. In order not to fall into any performance trap, my current practice is only use lodash as the only one util-lib in my JS projects.

stanleyxu2005 commented Aug 27, 2016

Regarding @coli and @bcherny 's experiment, if the code change is tiny and the performance is remarkable, I'd vote +1 to apply this change. This is a gratis improvement proposal for the angular team, seriously.

angular.copy = function(obj) {
  return JSON.parse(JSON.stringify(obj));
}

@gkalpak Improve performance of frequently use functions is a common sense. I feel a bit confused of using angular functions. If those functions are intended to be used internally, better rename foo to internal.foo or _foo. If we don't have much knowledge of angular's history, we never know these functions are not for public usage. In order not to fall into any performance trap, my current practice is only use lodash as the only one util-lib in my JS projects.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Aug 27, 2016

Contributor

@stanleyxu2005 JSON.parse(JSON.stringify(obj)) is very limited and does not work for the internal uses of angular.copy.

Contributor

jbedard commented Aug 27, 2016

@stanleyxu2005 JSON.parse(JSON.stringify(obj)) is very limited and does not work for the internal uses of angular.copy.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Aug 27, 2016

Member

every time you call angular.copy will create nested functions, which cost more memory and time.

@e-cloud, the question is whether the cost is neglible compared to other operations involved or not. If there is evidence that we can noticeably improve the performance of angular.copy (without sacrifizing functionality), we would be more than happy to do it.

Member

gkalpak commented Aug 27, 2016

every time you call angular.copy will create nested functions, which cost more memory and time.

@e-cloud, the question is whether the cost is neglible compared to other operations involved or not. If there is evidence that we can noticeably improve the performance of angular.copy (without sacrifizing functionality), we would be more than happy to do it.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Aug 27, 2016

Contributor

@e-cloud I tested your idea and it really makes no difference. It's only 3 closures per copy call. If it were 3 per object (and sub object) it would probably make a difference but that's not the case.

Contributor

jbedard commented Aug 27, 2016

@e-cloud I tested your idea and it really makes no difference. It's only 3 closures per copy call. If it were 3 per object (and sub object) it would probably make a difference but that's not the case.

@e-cloud

This comment has been minimized.

Show comment
Hide comment
@e-cloud

e-cloud Aug 29, 2016

@jbedard , Do you try it in scale?

Of source, in small scale it doesn't matter.

I've mention before, if only few calls.

e-cloud commented Aug 29, 2016

@jbedard , Do you try it in scale?

Of source, in small scale it doesn't matter.

I've mention before, if only few calls.

@jbedard jbedard self-assigned this May 3, 2018

@stripathix

This comment has been minimized.

Show comment
Hide comment
@stripathix

stripathix Jul 27, 2018

In my use case, I did not saw any a difference in angular.copy vs JSON.parse(JSON.stringfy(dataobject));

Rather _.map(dataobject, _.clone) was super fast.

angular.copy(dataobject)
2500ms

JSON.parse(JSON.stringfy(dataobject))
2500ms

_.map(dataobject, _.clone)
115ms

stripathix commented Jul 27, 2018

In my use case, I did not saw any a difference in angular.copy vs JSON.parse(JSON.stringfy(dataobject));

Rather _.map(dataobject, _.clone) was super fast.

angular.copy(dataobject)
2500ms

JSON.parse(JSON.stringfy(dataobject))
2500ms

_.map(dataobject, _.clone)
115ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment