Refactor shield functions:

[hjanetzek]

Some optimization to reduce the work of js-functios:

• Use text_source sequence for shield text instead of function.
• To discard shields when text is too long return no sprite instead of using an additional js call to filter them
• Don't allocate memory for building strings
• Don't spend cpu cycles on building strings, hashing and storing them.

[hjanetzek]

For our test-tile this change reduces the number of js calls to 3733 filter + 24930 style calls from previously 12633 filter + 27087 style function calls. https://gist.github.com/hjanetzek/addc9fd34a1d99e0621c07fb43bf48c0 https://gist.github.com/hjanetzek/07efe63492680f9f3831832966aa1dae And reduces js evaluation time on my duktape-optimizaiton branch from 47ms to 31ms

[bcamper]

I wasn't able to discern difference in performance loading this version in Tangram JS (but hey, it didn't get any worse either :)

[nvkelso]

(I haven't tested this visually yet, but will after some of the above are addressed.)

[bcamper]

I'd like to avoid any platform-specific implementation concepts leaking into the scene file like that.

On Wed, Dec 12, 2018 at 8:11 AM Hannes Janetzek notifications@github.com wrote:

@hjanetzek commented on this pull request.

In bubble-wrap-style.yaml https://github.com/tangrams/bubble-wrap/pull/274#discussion_r241007188:

                 draw:

mapzen_icon_library: priority: 46

• always show (or not show), irrespective of zoom

• visible: global.sdk_road_shields
• sprite: |
• function() {
• switch (feature.shield_text.length) {
• case 1: return 'US:I-1char';

This would still create new strings for each feature in js context which is quite expensive (at least it is in Duktape where all strings are interned, meaning the new string is hashed and then checked for identical strings to have only one instance in memory) What I thought of was that we could provide a stringbuilder function in js so that we could have a more efficient native implementation. This would require to return feature properties as proxy string objects instead of js strings, etc implementation details...

On the js side this could look like: ` return stringbuilder(feature.network, '-1char');

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/tangrams/bubble-wrap/pull/274#discussion_r241007188, or mute the thread https://github.com/notifications/unsubscribe-auth/AABBXRKKuz7AuDJr9yMfiX63IMieudeKks5u4QCFgaJpZM4ZJtLM .

[hjanetzek]

@bcamper What I meant was that Tangram can provide these helper functions for all implementations. Then we can recommend using stringbuilder() over + or [].join("")

[bcamper]

Yes I get it, but that seems very awkward to me.

On Wed, Dec 12, 2018 at 9:40 AM Hannes Janetzek notifications@github.com wrote:

@bcamper https://github.com/bcamper What I meant was that Tangram can provide these helper functions for all implementations. Then we can recommend using stringbuilder() over + or [].join("")

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/tangrams/bubble-wrap/pull/274#issuecomment-446610362, or mute the thread https://github.com/notifications/unsubscribe-auth/AABBXZwTq0kaJjnh1p4p32K5gUsvHTRvks5u4RVBgaJpZM4ZJtLM .

[hjanetzek]

There seem to be some stringbuilder implementation for js to avoid the overhead of the basic functions as well.

[hjanetzek]

Though it's certainly best to have no string building at all

[bcamper]

@hjanetzek I think I'm not following what you mean here, can you clarify with examples?

There seem to be some stringbuilder implementation for js to avoid the overhead of the basic functions as well.

If you suggest introducing a custom string function into the user-defined scene JS functions, that seems 1) like an non-ideal and not obvious exposure of internal implementation details to the user and 2) is motivated not just by ES but by implementation characteristics of the Duktape engine specifically (if I understand correctly). Further, implementing a no-op function wrapper like that for compatibility in Tangram JS could even have negative performance implications by adding another function call (though I suspect it wouldn't be significant in this case, it's another example of why I want to avoid tailoring like that in principle). Apologies if I'm misinterpreting your proposal though.

I just think we should look for more generic solutions in such cases. For example, since this kind of string substitution is pretty common, we could have dedicated syntax for it (e.g. following our tile URL template syntax):

sprite: "US:I-{feature.shield_text.length}char" (note double-quotes are just because of : inside YAML value)

Then we've got shorter scene syntax and more native control over how it's implemented. This follows the principle that the scene JS functions are ideal for truly custom logic, and for prototyping new features, but ultimately, we can provide native scene syntax for "core"/common needs. Note, I'm not saying we actually should add such syntax right now, without better context of the various tradeoffs in speed and file size (IMO the switch you proposed could be just fine, or maybe the prior dynamic version is) :)

[hjanetzek]

Ah right, a native syntax would be even better - I got distracted from thinking about this approach by too much Duktape optimization work..