Skip to content
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

Preset test performance #5832

Closed
3 of 4 tasks
bhousel opened this issue Jan 30, 2019 · 2 comments
Closed
3 of 4 tasks

Preset test performance #5832

bhousel opened this issue Jan 30, 2019 · 2 comments
Labels
performance Optimizing for speed and efficiency
Milestone

Comments

@bhousel
Copy link
Member

bhousel commented Jan 30, 2019

Test performance has really slowed a lot, and it's because of these lines

iD/modules/presets/index.js

Lines 134 to 155 in 17bbc3d

if (d.presets) {
var rawPresets = d.presets;
_forEach(d.presets, function(d, id) {
var existing = all.index(id);
if (existing !== -1) {
all.collection[existing] = presetPreset(id, d, _fields, visible, rawPresets);
} else {
all.collection.push(presetPreset(id, d, _fields, visible, rawPresets));
}
});
}
if (d.categories) {
_forEach(d.categories, function(d, id) {
var existing = all.index(id);
if (existing !== -1) {
all.collection[existing] = presetCategory(id, d, all);
} else {
all.collection.push(presetCategory(id, d, all));
}
});
}

What's going on here is a few things:

  • Every time we create a Context (almost every test), it will build the entire preset collection.
  • The preset collection is an array, which has gotten really big lately with the expanded name suggestion presets.
  • For each preset it is checking if it exists already. The check is done with _findIndex which is not very fast. This was added to support external presets in maprules service #5617

To attempt to speed the tests up, I tried adding this code to spec_helpers.js:

// run with a minimal set of presets for speed
iD.data.presets = {
    presets: {
        area: { name: 'Area', tags: {}, geometry: ['area'] },
        line: { name: 'Line', tags: {}, geometry: ['line'] },
        point: { name: 'Point', tags: {}, geometry: ['point'] },
        vertex: { name: 'Vertex', tags: {}, geometry: ['vertex'] },
        relation: { name: 'Relation', tags: {}, geometry: ['relation'] },
        // for tests related to areaKeys:
        building: { name: 'Building', tags: { building: 'yes' }, geometry: ['area'] }
    }
};

It does massively speed the tests up, but then several tests fail for interesting reasons.

So we should do a few things:

  • Consider optimizing presetCollection for faster access (switch it from Array to Object, and all related code?)
  • Figure out which parts of iD depend on the presets being a set up a certain way and try to avoid that.
  • Figure out which tests expect presets to be set up a certain way and set them up before the tests run
  • Update this spot of documentation which is not entirely true about the minimal set of presets that iD will run on.
@bhousel bhousel added the performance Optimizing for speed and efficiency label Jan 30, 2019
@bhousel
Copy link
Member Author

bhousel commented Jan 30, 2019

Updated:

Consider optimizing presetCollection for faster access (switch it from Array to Object, and all related code?)

Not going to do this now

Figure out which parts of iD depend on the presets being a set up a certain way and try to avoid that.

I changed a few things in 7138acc but we mostly handle this ok

Figure out which tests expect presets to be set up a certain way and set them up before the tests run

done!

Update this spot of documentation which is not entirely true about the minimal set of presets that iD will run on.

The documentation is actually correct, however some surprising test failures were because the tests expected areaKeys to work a certain way. When there are no presets, there are no areaKeys, and so features tagged like building=yes might also get a area=yes added to them, breaking tests. So I adjusted the areaKeys or presets in some situations so that the tests would work in a more realistic OSM-like way.

@bhousel bhousel closed this as completed Jan 30, 2019
@bhousel
Copy link
Member Author

bhousel commented Jan 30, 2019

done in 09f7f91 71b2d2c 7138acc 15c0b82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Optimizing for speed and efficiency
Projects
None yet
Development

No branches or pull requests

1 participant