refactor(BaseClass): reduces amount of created objects/functions #1613

stalniy · 2017-08-04T13:32:59Z

Utilizes prototype & Symbol instead of closure. This speeds up creation of overlays on map (markers, polygons, etc) and reduces amount of used memory.

Changes:

implemented EventEmitter completely on JS without DOM events
Moved all methods into prototype to reduce memory usage
hid variables and subscriptions under dynamic field name or Symbol if available
deperecated bindTo, replaced it with sync mehod
adds destroy which calls off and empty

wf9a5m75 · 2017-08-04T13:50:28Z

Please provide more information. The prof of reduce memory and improve speed up.
Also test code.

And why did you remove the hash code property?
You should not remove the property currently provided.

stalniy · 2017-08-04T13:55:15Z

I haven't removed hashCode:

Object.defineProperty(this, 'hashCode', { value: Math.floor(Date.now() * Math.random()) })

Update:

What do you mean by test code? If you are talking about unit testing, then where can I add them?
The proof. Previously BaseClass created on, one, off and bunch of other functions (in general 9 functions) each time you call BaseClass.apply(this). It takes time to create 9 functions for each new instance of those classes which inherit BaseClass. Currently this is:

www/BaseArrayClass.js:5:    BaseClass.apply(this);
www/TileOverlay.js:13:    BaseClass.apply(this);
www/GroundOverlay.js:11:  BaseClass.apply(this);
www/Circle.js:13:    BaseClass.apply(this);
www/Marker.js:14:    BaseClass.apply(this);
www/KmlOverlay.js:11:    BaseClass.apply(this);
www/Polyline.js:13:    BaseClass.apply(this);
www/Map.js:29:    BaseClass.apply(self);
www/Polygon.js:13:    BaseClass.apply(this);
www/HtmlInfoWindow.js:11:    BaseClass.apply(self);

So, the simplest calculation:
if you have 100 markers - this is 900 functions. If we take into consideration 1 map, HTMLInfoWindows for each marker then we have +901 function (correct if I'm wrong). In sum this is 1801 functions.
If it's possible not to create all that functions, why shouldn't we take this performance benefit for free?

Currently all functions were moved into prototype. That means they are reused across all instances. The last time I saw such optimization in knockout.js and at that time they gain ~50% of performance (objects created faster because code doesn't create functions in constructor, functions created only once).

wf9a5m75 · 2017-08-04T14:03:38Z

And also you should not change the method name, bindTo().
I designed this plugin similar with the Google Maps JavaScript v3.

And also did you test this code does not affect any current code, even multiple maps?
If you did, please provide how did you test?
This only JS code, and BaseClass effects or affects all classes, so you are trying to make a big change.

I believe you created safe test code a lot.

I can not accept your pull request soon. Please provide more information.
At least, I decline to change the name of method from bindTo to sync.

wf9a5m75 · 2017-08-04T14:31:10Z

The hashcode property is my mistake.

Did you create unit test, of course?
I asked please provide the unit test code you created.

BaseClass is just JS code. You can test only this class on browser, of Node.js.

stalniy · 2017-08-04T16:15:10Z

I did brief testing. This is the most simple event emitter. However I will add unit tests for BaseClass

wf9a5m75 · 2017-08-04T16:18:21Z

Test is really important especially for pull request.

In general (I'm not pointting only you),
from the sender a pull request side, if it is accepted, that's all.
But if it has a bug and reproduce is difficult, who has responsibility?
The person who accept the pull request, it is me.
In order to confirm and fix the bug, I need lots of spend time.

Especially you are trying to make a big change.
That's why I ask the proof (such as screen captures of memory usage of Chrome Developer tool and safari inspector), and unit test code.

Unit test has to include two side:

the proof of improvement
and the proof of safe that your change does not affect any current code

Thank you for your preparation.

stalniy · 2017-08-04T16:23:59Z

I know that it's important but I don't see any tests in this repository ;)

wf9a5m75 · 2017-08-04T16:29:46Z

I do in my local, but I can not share them. If you send me a more public test, I appreciate for you.

wf9a5m75 · 2017-08-04T16:39:55Z

By the way, you should not use the name as Cordova GoogleMaps plugin for iOS and Android (version 3.0).

Because you are not official. Since this plugin adapts the Apache License 2.0, I don't mind if you change the code, and redistribute it, but the plugin name is not good. This leads people who reached to your repo to misunderstand.

At least you should change the name if you want to use the version 3.0
https://github.com/stalniy/cordova-plugin-googlemaps

wf9a5m75 · 2017-08-04T16:54:05Z

And one more by the way.
Please don't misunderstand. I don't refuse your pull request.
I just worry about your pull request.
Improve pull request is appreciate.
That's why I open the full source code.

stalniy · 2017-08-04T17:17:48Z

I know but you should understand me as well: I don't want to spend a lot of time doing screenshots and calculating metrics. Better to spend it optimizing code, adding features or tests, real unit tests :)

wf9a5m75 · 2017-08-04T17:27:52Z

As I wrote, your code effect or affect the code overall.
Please recognize you are trying to make a big change to this plugin.
I understand you don't want to spend time, but I really worry about your pull request. Because the impact is so big.
That's why I'm asking.

If this pull request improves the performance significantly in safe, I will pay some amount for your work.
This is the correct way of the project donation. (Don't expect too much. The plugin project is poor.)

So what I want to know:

your code is safe or not,
how improve the performance

If you are tired today, you can do later.
I have a high expectation for your work ;)

wf9a5m75 · 2017-08-04T22:19:31Z

Git commit 02cc173, but I only replaced the BaseClass.js with yours in local.

I got an error with this simple code...

var div = document.getElementById("map_canvas");
var map = plugin.google.maps.Map.getMap(div);
map.one(plugin.google.maps.event.MAP_READY, function() {
  var htmlInfoWindow = new plugin.google.maps.HtmlInfoWindow();

  var html = [
    'This is &lt;b&gt;Html&lt;/b&gt; InfoWindow',
    '&lt;br&gt;',
    '&lt;button onclick="javascript:alert(\'clicked!\');"&gt;click here&lt;/button&gt;',
  ].join("");
  htmlInfoWindow.setContent(html);

  map.addMarker({
    position: {lat: 0, lng: 0},
    draggable: true
  }, function(marker) {

    marker.on(plugin.google.maps.event.MARKER_CLICK, function() {
      htmlInfoWindow.open(marker);
    });
    marker.trigger(plugin.google.maps.event.MARKER_CLICK);

  });
});

at HTMLInfoWindow.trigger (BaseClass.js:64)

at HTMLInfoWindow. (HtmlInfoWindow.js:170)

at HTMLInfoWindow.trigger (BaseClass.js:64)

at HTMLInfoWindow.calculate (HtmlInfoWindow.js:154)

at HTMLInfoWindow.trigger (BaseClass.js:64)

at Map. (HtmlInfoWindow.js:247)

at Map.js:648

at googlemaps-cdv-plugin.js:424

wf9a5m75 · 2017-08-04T22:31:11Z

And do not remove the addEventListener, addEventListenerOnce, and removeEventListener methods.

As I wrote, I designed this plugin is similar with the Google Maps JavaScript API v3.
The reason of existing on() and addEventListener() methods are I want to bring down the introduction step wall for beginners.

on() method is really popular, but also some pure JavaScript users prefer addEventListener().
So I decided I provide both, even it is alias.
Did you consider why I provide both methods?

I think I wrote to you before in another thread, please consider why this code is existed.
Keeping backward compatibility is really important for this plugin. Do not change the method name, do not remove methods please.
(Except very early implement, such as the method was implemented just one day ago.)

wf9a5m75 · 2017-08-04T22:36:04Z

Since you removed addEventListener(), this code does not work.

map.addEventListener(plugin.google.maps.event.MAP_READY, function() {

});

As you can see, if you think oh, I prefer on() instead of addEventListener(), let's remove it.
Then code is break.

Making a change to BaseClass means you are trying to make very big impact.

That's why I'm asking please make test code.

wf9a5m75 · 2017-08-04T23:03:58Z

I checked half of examples of the official demo app
As I tested, mostly fine except HTMLInfoWindow.

I haven't checked performance.
You described your code improve the performance , reduce memory usage.
Please make a proof.
For example, adding 1,000 markers, how many memory the current code, and your code?

Without the proof and fixing the error of HTMLInfoWindow, I can not accept your pull request.
In other word, I will accept you resolve the issue and make the proof.
Then I pay some amount for your work.

stalniy · 2017-08-05T04:59:52Z

fair enough. I will check that use case, add it to unit tests and will update PR. Thanks for the help with testing!

I will add back addEventListener stuff and bindTo

stalniy · 2017-08-05T05:15:13Z

@wf9a5m75 by the way, thanks for suggestion to pay me but I don't need this. Just include my nickname in release notes and this will be enough. Thanks

stalniy · 2017-08-05T05:48:41Z

I found the issue, it can be fixed on BaseClass level but I believe this is HTMLInfoWindow issue and should be fixed there.

Check https://github.com/mapsplugin/cordova-plugin-googlemaps/blob/multiple_maps/www/HtmlInfoWindow.js#L245

You will find:

self.on(event.INFO_OPEN);

So, the question why is on called without listener? Looks like a bug, am I right?

stalniy · 2017-08-05T06:20:31Z

I used newHtmlInfoWindow.html demo with 1000 markers (you can generate markers using http://www.geomidpoint.com/random/ + regexp find/replace to convert it to JSON).

Timeline for old BaseClass:

Timeline for refactored BaseClass:

As you can see Scripting part for refactored BaseClass is about on 400-500 ms lower.

Hope this is enought to proof that this code is faster.

Utilizes prototype & Symbol instead of closure

wf9a5m75 · 2017-08-05T16:58:13Z

Thank you for testing.
I just waked up, so I haven't checked this yet.

wf9a5m75 · 2017-08-06T00:21:59Z

I executed 16077 times of new Marker() (not map.addMarker).

This is current code.

And your code.

That's amazing! Almost 60% faster.
(Memory is actually increased though, not too much. So it's ok)

I will test all methods on Android and iOS.
Please wait for a while.

wf9a5m75 · 2017-08-06T00:24:11Z

And do you have a paypal account? If no, please tell me the best way to receive some amount for you?

Please send me the information to my e-mail address.

wf9a5m75 · 2017-08-06T01:47:55Z

www/BaseClass.js

+  },
+
+  get: function(key) {
+    return this[VARS_FIELD].hasOwnProperty(key) ? this[VARS_FIELD][key] : null;


This should be return this[VARS_FIELD].hasOwnProperty(key) ? this[VARS_FIELD][key] : undefined.
The null === undefined always false.

wf9a5m75 · 2017-08-06T06:56:13Z

I sent you some amount to your e-mail from my paypal account. Please follow the instruction.
Thank you for improving the cordova-googlemaps-plugin significantly.

wf9a5m75 · 2017-08-08T19:37:46Z

www/BaseClass.js

+    var index = topic.push(listener);
+    var self = this;
+
+    return function() {


Could you tell me why do you return this function?

function() { self.off(eventName, listener) }

I think this is not common way...

map.on(plugin.google.maps.event.MAP_READY, onMapReady)();

stalniy · 2017-08-08T20:45:47Z

it's much easier to work with subscriptions than with callbacks.

Lets imagine that you need to subscribe some function change event and afterwards when component or object is destroyed - unsubscribe it. Then you will do something like this:

class Component {
  constructor(input) {
     this.input = input
     this.changeListener = () => { console.log('changed!') }
     this.input.on('change', this.changeListener)
  }

  onDestroy() {
    this.input.off('change', this.changeListener)
  }
}

You need to store listener function in class instance. It's not very convenient especially when your function is big or just a wrapper for another function (e.g., (event) => { this.isClicked(event.pageX, event.pageY) }).

Instead it's more convenient (and more logically) to store reference to a subscription instance and eventually dispose subscription:

class Component {
  constructor(input) {
     this.input = input
     this.unsubscribe = this.input.on('change', () => { console.log('changed!') })
  }

  onDestroy() {
    this.unsubscribe()
  }
}

Code is cleaner.

Who uses such appoach:

rxjs (used by Angular 2+ and Ionic 2+)
knockoutjs

wf9a5m75 · 2017-08-08T21:05:58Z

This is not the way I designed.
Since your code blocks on my way, I can not choose to support Promise.

wf9a5m75 · 2017-08-08T21:08:13Z

In order to unsubscribe easily, I provide obj.off(), obj.off("eventName") and obj.off("eventName", callback).
This is enough.

wf9a5m75 · 2017-08-08T21:32:46Z

Sorry, but according to Google Trends, rxjs and knockoutjs are not so popular.

Past 12 months

https://trends.google.com/trends/explore?q=ionic,rxjs,knockoutjs,cordova

Past 5 years

https://trends.google.com/trends/explore?date=today%205-y&q=ionic,rxjs,knockoutjs,cordova

So if I am forced to choose make a choice, I definitely choose Promise because this is ionic way, and popular than rxjs and knockoutjs.

And Promise is almost work on every devices.
(One year ago was different. That's why I did not choose using Promise)

wf9a5m75 · 2017-08-08T21:34:47Z

Actually I have to make a choice to support the ionic-native/google-maps wrapper plugin currently.

stalniy · 2017-08-08T21:43:02Z

How do you want to use Promise (as I understood this relates to on and off methods somehow)?

I agree about knockout, but rxjs became popular when Angular2 was release and Ionic2 is built on top of Angular2.

wf9a5m75 · 2017-08-08T22:31:07Z

The problem of the wrapper plugin is which uses then and subscribe.

For example:

this.map.one(GoogleMapsEvent.READY, () => {

   for (let i = 0; i < locations.length; i++) {
     this.map.addMarker(locations[i], this.onMarkerAdded);
   }

});

onMarkerAdded(marker: Marker) {
  marker.on(GoogleMapsEvent.MARKER_CLICK, this.onMarkerClick);
}

onMarkerClick(...params: any[]) {
  let latLng = params[0];
  let marker = params[1];
  marker.showInfoWindow();
}

In order to use write like the above code, on and one have to return the object of the class of the wrapper plugin.

But on and one of the wrapper plugin passes the raw values to the event lister.

Currently, the JS plugin passes the marker instance for all marker events.
It is not written in the document though.
Because ionic does not allow to use this to obtain the caller object of event listener.

Ionic does not allow this way

marker1.on(GoogleMapsEvent.MARKER_CLICK, onMarkerClick);
marker2.on(GoogleMapsEvent.MARKER_CLICK, onMarkerClick);

onMarkerClick() => {
  // there is no way to detect which marker called this event listener
}

In order to solve this problem, the plugin passes the instance as the second argument.
(But current ionic wrapper plugin does not receive it though. I'm patching.)

goal

marker1.on(GoogleMapsEvent.MARKER_CLICK, onMarkerClick);
marker2.on(GoogleMapsEvent.MARKER_CLICK, onMarkerClick);

onMarkerClick(...params: any[]) => {
  let latLng = params[0];
  let marker = params[1];
}

However, this plugin does not support Promise (browser native) and Observable (rxjs),
the wrapper plugin has to convert the instance type from pure JS to the ionic class,
something like this (this code might not work, just example):

  on(eventName: string): Observable<any> {

    return  new Observable((observer) => {
         this._objectInstance.on(eventName, (...results: any[]) {
           for (let i = 0; i < results.length; i++) {
             if (results[i] instanceof GoogleMaps.getPlugin().BaseClass) {
               results[i] = convertFromJS_to_Ionic(results[i]));
             }
           }
           observer.next.apply(observer, results);
         });
       }
     );
     return obs;
  }

Otherwise, this code would become like this:

marker1.on(GoogleMapsEvent.MARKER_CLICK, onMarkerClick);
marker2.on(GoogleMapsEvent.MARKER_CLICK, onMarkerClick);

onMarkerClick(...params: any[]) => {
  let latLng = params[0];
  let marker = params[1];

  // This does not work, because marker is instance of JS, not ionic wrapper's one.
  // marker.one(GoogleMapsEvent.INFO_CLICK).then(() => {
  //   alert("info clicked");
  // });

  // This works, but this leads many people confuse a lot.
  marker.one(GoogleMapsEvent.INFO_CLICK, () => {
    alert("info clicked");
  });
}

So I'm planing to use the Promise and Observable if these both classes are available.
If the JS side supports them, the ionic wrapper plugin does not have to convert the instance.

marker1.on(GoogleMapsEvent.MARKER_CLICK, onMarkerClick);
marker2.on(GoogleMapsEvent.MARKER_CLICK, onMarkerClick);

onMarkerClick(...params: any[]) => {
  let latLng = params[0];
  let marker = params[1];

  // This code works if the JS side support Promise and Observable, even if the marker instance is JS instance.
  marker.one(GoogleMapsEvent.INFO_CLICK).then(() => {
     alert("info clicked");
  });
}

The ionic wrapper plugin keeps the same method names as the JS plugin.
So you don't need to keep in mind the instance is JS or ionic one.

I need to detect the Observable is available or not, but if the JS plugin supports Promise and Observable both, the benefit is big for ionic users.
And reduce problems for me.

Hence I need to return something:

  on: function(eventName, listener) {
    this[SUBSCRIPTIONS_FIELD][eventName] = this[SUBSCRIPTIONS_FIELD][eventName] || [];
    var topic = this[SUBSCRIPTIONS_FIELD][eventName];
    var index = topic.push(listener);
    var self = this;

    if (checkAvailabilityObservable()) {
       return new Observable(...);
    } else {
       return this;
    }
  },

And you may notice that the JS plugin currently returns their instance of the class.

https://github.com/mapsplugin/cordova-plugin-googlemaps/blob/multiple_maps/www/Marker.js#L231

Marker.prototype.setFlat = function(flat) {
    flat = common.parseBoolean(flat);
    this.set('flat', flat);
    return this;
};
Marker.prototype.setIcon = function(url) {
    if (url && common.isHTMLColorString(url)) {
        url = common.HTMLColor2RGBA(url);
    }
    this.set('icon', url);
    return this;
};

I designed this allows you to use the chain method like jquery.

marker.setTitle("HelloWorld").showInfoWindow();

I forgot to return this in the on method, but I prefer this way.

marker.on(plugin.google.maps.event.MARKER_CLICK, onMarkerClick)
            .on(plugin.google.maps.event.INFO_CLICK, onInfoWndClick);

I understand your point, but as I write previously, this plugin supports three ways obj.off(), obj.off("eventName"), and obj.off("eventName", callback).

You just keep the callback if you want to remove particular event listener.

class Component {
  constructor(input) {
     this.input = input
     this.change = () => { console.log('changed!') };
     this.input.on('change', this.change);
  }

  onDestroy() {
    this.input.off('change', this.change);
  }
}

or simply just off() or destroy (you added)

class Component {
  constructor(input) {
     this.input = input
     this.input.on(() => { console.log('changed!') });
  }

  onDestroy() {
    this.input.destroy();
  }
}

wf9a5m75 · 2017-08-08T22:35:57Z

And this code create different closures every time.

return function() {
      self.off(eventName, listener);
    };

It means if you create 1,000 markers, this code creates another 1,000 closures.

Did you say this is bad performance, didn't you?

stalniy · 2017-08-08T22:50:27Z

ok, it makes sense

wf9a5m75 added need more information enhancement labels Aug 4, 2017

wf9a5m75 added the pending / waiting label Aug 4, 2017

refactor(BaseClass): reduces amount of created objects/functions

8b87931

Utilizes prototype & Symbol instead of closure

wf9a5m75 reviewed Aug 6, 2017

View reviewed changes

wf9a5m75 merged commit 8b87931 into mapsplugin:multiple_maps Aug 6, 2017

wf9a5m75 reviewed Aug 8, 2017

View reviewed changes

wf9a5m75 mentioned this pull request Aug 9, 2017

Fix: Google Maps Doc page danielsogl/awesome-cordova-plugins#1834

Merged

refactor(BaseClass): reduces amount of created objects/functions #1613

refactor(BaseClass): reduces amount of created objects/functions #1613

Conversation

stalniy commented Aug 4, 2017 • edited Loading

wf9a5m75 commented Aug 4, 2017

stalniy commented Aug 4, 2017 • edited Loading

wf9a5m75 commented Aug 4, 2017

wf9a5m75 commented Aug 4, 2017

stalniy commented Aug 4, 2017

wf9a5m75 commented Aug 4, 2017

stalniy commented Aug 4, 2017

wf9a5m75 commented Aug 4, 2017

wf9a5m75 commented Aug 4, 2017 • edited Loading

wf9a5m75 commented Aug 4, 2017 • edited Loading

stalniy commented Aug 4, 2017

wf9a5m75 commented Aug 4, 2017 • edited Loading

wf9a5m75 commented Aug 4, 2017 • edited Loading

wf9a5m75 commented Aug 4, 2017

wf9a5m75 commented Aug 4, 2017

wf9a5m75 commented Aug 4, 2017

stalniy commented Aug 5, 2017

stalniy commented Aug 5, 2017

stalniy commented Aug 5, 2017

stalniy commented Aug 5, 2017 • edited Loading

wf9a5m75 commented Aug 5, 2017

wf9a5m75 commented Aug 6, 2017

wf9a5m75 commented Aug 6, 2017

wf9a5m75 Aug 6, 2017

Choose a reason for hiding this comment

wf9a5m75 commented Aug 6, 2017

wf9a5m75 Aug 8, 2017

Choose a reason for hiding this comment

stalniy commented Aug 8, 2017

wf9a5m75 commented Aug 8, 2017 • edited Loading

wf9a5m75 commented Aug 8, 2017

wf9a5m75 commented Aug 8, 2017 • edited Loading

wf9a5m75 commented Aug 8, 2017 • edited Loading

stalniy commented Aug 8, 2017 • edited Loading

wf9a5m75 commented Aug 8, 2017 • edited Loading

wf9a5m75 commented Aug 8, 2017

stalniy commented Aug 8, 2017

stalniy commented Aug 4, 2017 •

edited

Loading

stalniy commented Aug 4, 2017 •

edited

Loading

wf9a5m75 commented Aug 4, 2017 •

edited

Loading

wf9a5m75 commented Aug 4, 2017 •

edited

Loading

wf9a5m75 commented Aug 4, 2017 •

edited

Loading

wf9a5m75 commented Aug 4, 2017 •

edited

Loading

stalniy commented Aug 5, 2017 •

edited

Loading

wf9a5m75 commented Aug 8, 2017 •

edited

Loading

wf9a5m75 commented Aug 8, 2017 •

edited

Loading

wf9a5m75 commented Aug 8, 2017 •

edited

Loading

stalniy commented Aug 8, 2017 •

edited

Loading

wf9a5m75 commented Aug 8, 2017 •

edited

Loading