Refactor shield functions: #274

hjanetzek · 2018-12-08T16:25:30Z

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.

- Return no sprite when text is too long to discard them 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.

- Don't keep comments in JS code - we have to parse the source strings and keep them around. When possible use yaml comments.

hjanetzek · 2018-12-08T17:22:06Z

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

- can be enabled for debugging to see which shields were discarded by the sprite functions

bcamper · 2018-12-09T23:11:00Z

bubble-wrap-style.yaml

-            }
-            */
-        }
+    ux_language_text_source_road_ref_and_name: global.ux_language_text_source


Definitely better to just reuse the existing function like this where possible, but this also reminded me, I added a simple cache to JS recently to reuse scene JS functions internally when they have the same source (similar to caching shader programs by source), and it can help catch cases like this.

Seems worth a separate Tangram ES issue... @hjanetzek @matteblair?

We compile identical functions only one time (per js context) as well :)

bcamper · 2018-12-09T23:13:07Z

bubble-wrap-style.yaml

-                                    return feature.ref;
-                                }
-                            }
+                        text_source: [shield_text, ref]


Native scene syntax FTW!!! 👌

So, it turns out shield_text != ref, and more specifically shield_text.length() != ref.length().

It looks like you've accounted for that in the new sprite function (above), though, so this logic would always be coherent (modulo my 1 comment about stopping stripping spaces from ref above).

bcamper · 2018-12-09T23:14:43Z

bubble-wrap-style.yaml

                        # you need to match any custom shield to the vector tile `network` values
-                        sprite: function() { return (feature.network + '-' + feature.shield_text.length + 'char'); }
-                        # FYI: sprite_default doesn't support functions, default is carried by parent style's sprite function
+                        sprite_default: |


Note sprite_default doesn't support functions, I think this logic was moved up to the top of the shield layer now?

Yeah, this section doesn't look right. Shouldn't it still have the sprite: function included?

bcamper · 2018-12-09T23:22:03Z

bubble-wrap-style.yaml

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


This switch-case style is great for these.

Hmm. That's may be fine to expand just for this USA example here with switch statements... but seems rather excessive to bloat the full USA and international shield theme imports. Those could easily get 5x as large as they are today in terms of line count. It's not impossible since much of that is auto-generated, but my goodness!

If the switch is more performant... could we say instead:

case 1: return feature.network + '-1char'; case 2: return feature.network + '-2char'; case 3: return feature.network + '-3char'; case 4: return feature.network + '-4char'; case 5: return feature.network + '-5char';

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 from the getter of our feature proxy object, etc implementation details...

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

Edit: Oh - the string proxy would be an additional optimization to not have to copy into and intern 'feaure.network' in js context. The stringbuilder could be done without it.

bcamper · 2018-12-10T03:32:32Z

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 · 2018-12-12T01:14:55Z

bubble-wrap-style.yaml

@@ -14,57 +14,95 @@ global:
    #ux/ui
    ux_language: false             # l10n language code, trusting OSM in v0.10 tiles, fixed in v1.0 tiles
    ux_language_fallback: false    # l10n language code, trusting OSM in v0.10 tiles, fixed in v1.0 tiles
+    # if a ux_langauge has been defined use that, else if there is feature name


Nit: add line space above here as it's getting a little crowded to read now.

nvkelso · 2018-12-12T01:22:29Z

bubble-wrap-style.yaml

-            */
-        }
+    ux_language_text_source_road_ref_and_name: global.ux_language_text_source
+    # ux_language_text_source_road_ref_and_name: |


I'm okay removing the commented out ux_language_text_source_road_ref_and_name section – it's vestigial from when Tangram didn't really support road shields.

nvkelso · 2018-12-12T01:23:55Z

bubble-wrap-style.yaml

+    #         }
+    #     }
+    ux_language_text_source_road_ref_and_name_short: global.ux_language_text_source
+    # ux_language_text_source_road_ref_and_name_short: |


I'm okay removing the commented out ux_language_text_source_road_ref_and_name_short section – it's vestigial from when Tangram didn't really support road shields.

nvkelso · 2018-12-12T01:25:51Z

bubble-wrap-style.yaml

+    #                   (global.ux_language_fallback && feature['name:left:'+global.ux_language_fallback]) ||
+    #                   feature['name:left'];
+    #        if( right && left ) {
+    #            if( right.includes(' ') || left.includes(' ') ) {


Removing this if/else logic for stacking or not stacking the resulting ux_language_text_source_boundary_lines text string would be a visual regression.

nvkelso · 2018-12-12T01:28:08Z

bubble-wrap-style.yaml

    ux_language_text_source_piste_advanced: |
        function() {
-            var name = (global.ux_language && feature['name:'+global.ux_language]) || (global.ux_language_fallback && feature['name:'+global.ux_language_fallback]) || feature['name'];
+            var name = (global.ux_language && feature['name:'+global.ux_language]) ||
+                       (global.ux_language_fallback && feature['name:'+global.ux_language_fallback]) || feature['name'];


nit: The convention above in this PR is to separate each || onto a new line, I think feature['name']; here as well?

nvkelso · 2018-12-12T01:28:24Z

bubble-wrap-style.yaml

            return name ? ('◆ ' + name) : '◆';
        }
    ux_language_text_source_piste_expert: |
        function() {
-            var name = (global.ux_language && feature['name:'+global.ux_language]) || (global.ux_language_fallback && feature['name:'+global.ux_language_fallback]) || feature['name'];
+            var name = (global.ux_language && feature['name:'+global.ux_language]) ||
+                       (global.ux_language_fallback && feature['name:'+global.ux_language_fallback]) || feature['name'];


nit: The convention above in this PR is to separate each || onto a new line, I think feature['name']; here as well?

nvkelso · 2018-12-12T01:28:29Z

bubble-wrap-style.yaml

            return name ? ('◆◆ ' + name) : '◆◆';
        }
    ux_language_text_source_building_and_address: |
        function() {
-            var name = (global.ux_language && feature['name:'+global.ux_language]) || (global.ux_language_fallback && feature['name:'+global.ux_language_fallback]) || feature['name'];
+            var name = (global.ux_language && feature['name:'+global.ux_language]) ||
+                       (global.ux_language_fallback && feature['name:'+global.ux_language_fallback]) || feature['name'];


nit: The convention above in this PR is to separate each || onto a new line, I think feature['name']; here as well?

nvkelso · 2018-12-12T01:29:24Z

bubble-wrap-style.yaml

@@ -2191,6 +2234,7 @@ layers:
                            stroke: { color: [1.00,1.00,1.00], width: 2 }

        shields:
+            enabled: global.sdk_road_shields


Logically this makes sense, but I recall "reasons" for why that was backed out to be a filter instead. I'll need to review the old PRs...

(It might be related to a fix in Tangram v0.16 for reevaluating globals?)

nvkelso · 2018-12-12T01:35:55Z

bubble-wrap-style.yaml

+                          if (text === undefined) {
+                            text = feature.ref;
+                            if (text === undefined) { return undefined; }
+                            if (text.length > 3) {


This should only be done if the text is coming from ref not shield_text, as shield_text deliberately sometimes keeps whitespace in the value. (So this logic is good.)

But later on below... since you propose to set text_source: [sheild_text, ref], the symbol size will no longer match after you modify the interpreted value of ref here. So we either need to stop stripping the space from ref here, or add the stripping to the text source below. I'd be fine stopping stripping of the space from ref here as the vector data is soooo much better starting in Tilezen v1.5 tiles.

nvkelso · 2018-12-12T01:49:06Z

bubble-wrap-style.yaml

-                    mapzen_icon_library:
-                        # this is sensitive to values > 56
-                        priority: 56
-                early:


This moved to:

https://github.com/tangrams/bubble-wrap/pull/274/files#diff-d5306f7f58640a806f55ed4eda76d550R2288

nvkelso · 2018-12-12T01:49:34Z

bubble-wrap-style.yaml

                draw:
                    mapzen_icon_library:
                        # this is sensitive to values > 56
                        priority: 56
-                early:


This moved to:

https://github.com/tangrams/bubble-wrap/pull/274/files#diff-d5306f7f58640a806f55ed4eda76d550R2291

nvkelso · 2018-12-12T01:51:39Z

bubble-wrap-style.yaml

@@ -2215,15 +2258,40 @@ layers:
            draw:
                mapzen_icon_library:
                    # you need to match any custom shield to the vector tile `network` values
+                    # sprite: |


Can just remove this commented out section?

nvkelso · 2018-12-12T01:51:53Z

bubble-wrap-style.yaml

+                    #       return ['generic_shield-', feature.ref.length, 'char'].join('');
+                    #     }
+                    ## [].join('') is good - Better build no new strings at all:
+                    sprite_default: ~ # remove 'generic' default to make discard label-by-sprite function work


Don't need this about sprite_default at all now?

(Or at least I don't understand what the ~ does in YAML / scene files.)

nvkelso · 2018-12-12T01:52:51Z

bubble-wrap-style.yaml

@@ -2239,19 +2307,12 @@ layers:
                        - [13, 1.50]
                        - [14, 2.0]
                    cull_from_tile: true
-                    visible: false
+                    visible: true


Turning this on by default in the past has caused too many shields to show up. We've cleaned up shields server side quite a bit since then, so worth reevaluating. But this may cause visual problems.

nvkelso · 2018-12-12T01:55:01Z

bubble-wrap-style.yaml

@@ -2332,7 +2389,6 @@ layers:
                    mapzen_icon_library:
                        priority: 55
                        #color: green


Might as well take this debug line out, too? ;)

nvkelso · 2018-12-12T01:57:03Z

bubble-wrap-style.yaml

-                       mapzen_icon_library:
-                           visible: false
-
-            # this is kinda a hack – generally we don't have artwork sized 6+


This is implied now – in that no sprite is defined for cases where the size is not in the range 1-5.

nvkelso · 2018-12-12T01:58:12Z

bubble-wrap-style.yaml

+                    all:
+                        - not:
+                            kind: [minor_road]
+                            network: ['US:I','US:US','US:US:Business','US:US:Truck','US:US:Alternate'] # propably all US:* - bug below?


I think the old logic's purpose was to just change the priority, not limit it to specific kinds or networks. So remove these lines but keep the function on the text length below?

nvkelso · 2018-12-12T02:00:39Z

bubble-wrap-style.yaml

@@ -2446,37 +2436,154 @@ layers:
                        - shield_text: true
                draw:
                    mapzen_icon_library:
-                        #texture: mapzen_icon_library_shields_usa
+                        # texture: mapzen_icon_library_shields_usa


This line should just be removed.

nvkelso · 2018-12-12T02:01:47Z

bubble-wrap-style.yaml

+                            case 3:
+                                return 'generic_shield-3char';
+                            case 4:
+                                if ($zoom < 11) break;


nvkelso · 2018-12-12T02:05:19Z

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

bcamper · 2018-12-12T13:42:52Z

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 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bubble-wrap-style.yaml <#274 (comment)>: > 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 <#274 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABBXRKKuz7AuDJr9yMfiX63IMieudeKks5u4QCFgaJpZM4ZJtLM> .

hjanetzek · 2018-12-12T14:40:01Z

@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 · 2018-12-12T14:41:10Z

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

…

On Wed, Dec 12, 2018 at 9:40 AM Hannes Janetzek ***@***.***> 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 <#274 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABBXZwTq0kaJjnh1p4p32K5gUsvHTRvks5u4RVBgaJpZM4ZJtLM> .

hjanetzek · 2018-12-12T14:42:29Z

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

hjanetzek · 2018-12-12T14:53:38Z

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

bcamper · 2018-12-12T15:11:33Z

@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 · 2018-12-12T15:22:41Z

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

hjanetzek added 3 commits December 8, 2018 16:15

Refactor sprite functions:

f283337

- Return no sprite when text is too long to discard them 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.

Add back special 4-5 char priority filter

ccf6081

Move visible: global.sdk_road_shields to overall 'shields' layer

a11dfe7

hjanetzek changed the title ~~Refactor sprite functions:~~ Refactor shield functions: Dec 8, 2018

global.ux_language_text_source formatting and cleanups

1b7e6fb

- Don't keep comments in JS code - we have to parse the source strings and keep them around. When possible use yaml comments.

hjanetzek added 2 commits December 8, 2018 18:57

Fix duplicate US-US:

cd16294

Unset 'generic' sprite fallback for shields

3d56897

- can be enabled for debugging to see which shields were discarded by the sprite functions

bcamper reviewed Dec 9, 2018

View reviewed changes

nvkelso reviewed Dec 12, 2018

View reviewed changes

bubble-wrap-style.yaml

case 3:

return 'generic_shield-3char';

case 4:

if ($zoom < 11) break;

Copy link

Member

nvkelso Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Refactor shield functions: #274

Are you sure you want to change the base?

Refactor shield functions: #274

Conversation

hjanetzek commented Dec 8, 2018 • edited Loading

hjanetzek commented Dec 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvkelso Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvkelso Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjanetzek Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

bcamper commented Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvkelso Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvkelso Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvkelso commented Dec 12, 2018

bcamper commented Dec 12, 2018 via email

hjanetzek commented Dec 12, 2018

bcamper commented Dec 12, 2018 via email

hjanetzek commented Dec 12, 2018 • edited Loading

hjanetzek commented Dec 12, 2018

bcamper commented Dec 12, 2018 • edited Loading

hjanetzek commented Dec 12, 2018

hjanetzek commented Dec 8, 2018 •

edited

Loading

hjanetzek commented Dec 8, 2018 •

edited

Loading

nvkelso Dec 12, 2018 •

edited

Loading

nvkelso Dec 12, 2018 •

edited

Loading

hjanetzek Dec 12, 2018 •

edited

Loading

bcamper commented Dec 10, 2018 •

edited

Loading

nvkelso Dec 12, 2018 •

edited

Loading

nvkelso Dec 12, 2018 •

edited

Loading

hjanetzek commented Dec 12, 2018 •

edited

Loading

bcamper commented Dec 12, 2018 •

edited

Loading