Move bench under `test` folder #979

HarelM · 2022-02-10T15:23:27Z

Launch Checklist

This PR moves bench folder under test where I believe it should be.
I've ran the bench locally but I'm not 100% sure it is working as expected.
@wipfli @birkskyum can you guys make sure I didn't break anything here?

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.

github-actions · 2022-02-10T15:31:25Z

Bundle size report:

Size Change: 0 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file	Before	After	Change
maplibre-gl.js	185 kB	185 kB	0 B
maplibre-gl.css	9.43 kB	9.43 kB	0 B

ℹ️ View Details

No major changes

test/bench/run-benchmarks.ts

birkskyum · 2022-02-10T15:38:40Z

It fits better there for sure.

birkskyum

LGTM. Only the paths in git workflow (upload-benchmarks.yml) left to update.

HarelM · 2022-02-10T16:02:56Z

Updated. Thanks for the review!
When I opened the browser I saw some errors related to source_cache and some warning about a missing icon.
Is this expected?

test/bench/README.md

wipfli · 2022-02-10T16:52:16Z

Tested running benchmarks locally and they worked the same way as they used to. Please fix link in readme. Other than that, looks good to me.

wipfli · 2022-02-10T16:53:13Z

When I opened the browser I saw some errors related to source_cache and some warning about a missing icon.
Is this expected?

I don't know. Checkout main and compare how it was before.

birkskyum

Found these paths as well that I think needs attention.

Oh... this breaks (http://localhost:9966/test/bench/versions#FilterEvaluate) - the benchmarks that are already generated and uploaded to github have paths like shown in the image i uploaded, and even though we update the paths, that will only update the new builds, so this is actually not backwards compatible unless we do one of the following:

Forward calls to /bench/ to /test/bench,
Make the server launch from /test instead of /, so the paths would stay localhost/bench, and hope the script doesn't make http requests to resources outside of /test
Rebuild the old benchmark builds with new paths...

wipfli · 2022-02-10T17:41:12Z

Please also make sure that npm run benchmark completes all benchmarks successfully.

HarelM · 2022-02-10T17:51:03Z

I definitely don't like the backward compatibility need here. This is counter productive when talking about refactoring and moving files and other things around.
The fact that we use code that is not address agnostic is just bad practice.
I don't know how to properly solve this, but it's just another sign (besides the need to upload js files for every version) that this is poorly written... :-(
I'll open an issue about it...

birkskyum · 2022-02-10T17:56:27Z

I agree, it's really annoying... tripped me the other day with the es modules as well, and I'm thinking it might be more reasonable for us to upload the maplibre-gl-min.js build instead of the benchmarks build, because do we even need a such build, or is a dev server enough.

birkskyum · 2022-02-10T18:05:52Z

We have all the old versions here already: https://cdn.skypack.dev/maplibre-gl@^1.3, so we could use the releases, and then the current branch, and forget about "main" in the comparisons...

HarelM · 2022-02-10T18:42:03Z

See #982.
I think we should use the regular build artifacts and not this crazy complicated bundling and previous version uploaded artifacts...
Even at the cost of less performance coverage.
I'm not sure, I tend to say that this PR should be closed and we should focus on a better solution going forward.

HarelM · 2022-02-11T07:38:28Z

I added a start-server-at-test to fix the issue with the relative path, but now the css file is not available since dist folder is not part of this server. I can hack that too I guess, but I don't know if this is the right direction as I said before.
We also have a css from mapbox that we are loading as part of the benchmarks with I believe we should stop using:
<link href='https://www.mapbox.com/base/latest/base.css' rel='stylesheet' />
I'm not sure what it does, but it is used in the styles benchmark.

birkskyum · 2022-02-11T08:14:11Z

How about if we just use this for the css? https://unpkg.com/maplibre-gl@latest/dist/maplibre-gl.css - if it makes sense to load the css at all?

HarelM · 2022-02-11T08:31:06Z

Seems like the tests are running without loading the css, so I tend to say that it's redundant...
I need to see if removing the mapbox css is possible too.

birkskyum · 2022-02-11T08:33:36Z

Sure, the gl-stats.ts pull in the css from /dist/ , so that still works

HarelM · 2022-02-11T09:05:48Z

I don't see that gl-stat uses css, it pull the code and replaces that when running, but that's a different story.
In any case, I think this PR is read.
I'm not super exited about it, but it's better than what it used to be I believe.
I don't think it breaks anything as far as I tested.
Feel free to test this too as I'm not super familiar with the benchmarks usage...

birkskyum · 2022-02-11T09:39:06Z

Great! I'm almost through the whole benchmarks.

birkskyum

Same performance as main, so it lgtm!

xabbu42 · 2022-02-15T16:22:00Z

@birkskyum just as a side node: the posted benchmark results have several values with low correlation (marked by ☠). these results should not be trusted (see https://github.com/maplibre/maplibre-gl-js/tree/main/test/bench#interpreting-benchmark-results).

* Migrate expression tests to jest (#965) * Revert "Move benchmarks to ES modules (#964)" (#969) This reverts commit aa8ed9d. * Migrate query tests from puppeteer to playwright (#966) * Fix web worker in watch mode (#968) * Fix web worker in watch mode * Add webworker support throughout * Don't change rollup for style-spec and benchmarks * Revert "Don't change rollup for style-spec and benchmarks" This reverts commit 62a7e14. * Remove duplicate node-resolve configuration * Simplify build pipeline (#961) * Remove build-tsc compile step * re-enable failing style-spec test * cleanup * cleanup * fix style-spec-test * Create codeql-analysis.yml * Bump simple-get from 3.1.0 to 3.1.1 (#971) Bumps [simple-get](https://github.com/feross/simple-get) from 3.1.0 to 3.1.1. - [Release notes](https://github.com/feross/simple-get/releases) - [Commits](feross/simple-get@v3.1.0...v3.1.1) --- updated-dependencies: - dependency-name: simple-get dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump nanoid from 3.1.30 to 3.2.0 (#973) Bumps [nanoid](https://github.com/ai/nanoid) from 3.1.30 to 3.2.0. - [Release notes](https://github.com/ai/nanoid/releases) - [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md) - [Commits](ai/nanoid@3.1.30...3.2.0) --- updated-dependencies: - dependency-name: nanoid dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump cached-path-relative from 1.0.2 to 1.1.0 (#972) Bumps [cached-path-relative](https://github.com/ashaffer/cached-path-relative) from 1.0.2 to 1.1.0. - [Release notes](https://github.com/ashaffer/cached-path-relative/releases) - [Commits](https://github.com/ashaffer/cached-path-relative/commits) --- updated-dependencies: - dependency-name: cached-path-relative dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Move browser test to playwright (#974) * Bump ts-node for 3x faster postinstall (#975) * Remove web worker replacement (#976) * Attribution default open for osm (#795) * Update CHANGELOG.md * Revert "Update CHANGELOG.md" This reverts commit 0b81a41. * attribution fixes (from astridx) a1272d9 b9b0370 * Update .gitignore * fix missing new line lint complained about * Revert "attribution fixes (from astridx)" This reverts commit 2031d8e. * Default compact to be open by default Default compact button to be open by default, to make OpenStreetMap attibutions to show by default like mentioned in ( #205 ) This uses an idea from ( #205 (comment) ) to show compact open by default, but close it when dragged. * remove test string I forgot to remove * fix compact button showing when there are no attributions * update whitespace trim to es6 syntax * fixes after testing various states of compact * revert back so it is open when starting in fullscreen * revert .gitignore * fix lint errors * fix tests Most of these fails because I made the compact button not show if attributes are empty. The other test fail because when switching between >640 and <=640 the open attribute is now added * lint * Update package-lock.json * Update package-lock.json * Update CHANGELOG.md * delete problematic files * put back deleted files * test/integration/assets/sprites/icon-text-fit-1x@2x.json * restore files Co-authored-by: acalcutt <acalcutt@worcester.edu> * Fix benchmarks (#984) * Move bench under `test` folder (#979) * Add typeof guard to performance variable (#986) * Fixes #768 * Fix comment and add changelog * Release 2.1.2 Version (#987) * Release 2.1.2 Version * Fix changelog according to comments * Fix release.yml due to bench folder move (#988) * lint function-url-quotes/ (#983) * Bump to v2.1.3 - Fix postinstall error `ts-node` not found in non-dev installs (#991) * Bump to v2.1.4 - Fix missing `postinstall.js` file in npm publish (#992) * Bump to v2.1.5-pre.1 - Publish empty `postinstall.js` file (#994) * Bump to v2.1.5 (#996) * Bump to v2.1.6-pre.1 - Fix `dist/package.json` (#998) * Bump to v2.1.6 (#999) * prefix (#1004) * correct done (#1006) * Add adjustment to rendering glyphs (#1005) * add topAdjustment for glyph * add topAdjustment for glyph * update changelog * add issue number * add alphabet and cjk text test #1002 * fix lint error * Simplify render tests (#1003) Fixes #1008 * Initial commit - half of the tests are failing * Missing file * Remove template rendering * Fix usage of localizeUrls * Move harness and server to render folder, simplify render code to be sepcific. * Move function outside and add some typings * lint * Add typings, move compare results outside * Remove server usage and replace it by file system reading. * Move xhr mocking to main flow * Move functions outside the render function * Move creation of tests to render.ts, added more typings * Move suite implementation into main flow file, add some typings. * lint * Moved all logic to a single file that runs the render tests * remove color from harness * Remove jest run command * Small fixes * lint * Cleanup * More cleanup * Remove harness and move it to a single file. Remove the jest version. * Remove ignore related code and tests * Remove ignore, add types * Add more types * lint * Migrate actual image generation to not use Buffer * lint * Fix CI run * Fix CI properly this time... * Revert png change * Revert only buffer usage * Revert png and buffer * Simplify png in mock file * Move to use uint8array and change throshold * Added diff info * remove specific threshold from test as it fails * Fix typo * Fix according to code review * Husky pre-commit do not fix lint (#1019) * Fix css * Use main package-lock.json Co-authored-by: Birk Skyum <74932975+birkskyum@users.noreply.github.com> Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Harel M <harel.mazor@gmail.com> Co-authored-by: Andrew Calcutt <acalcutt@techidiots.net> Co-authored-by: acalcutt <acalcutt@worcester.edu> Co-authored-by: vanilla-lake <95251033+vanilla-lake@users.noreply.github.com> Co-authored-by: Astrid <astridx@users.noreply.github.com> Co-authored-by: Kanahiro Iguchi <mediterranean1769@gmail.com>

xabbu42 · 2022-03-10T10:15:33Z

Sorry to be this late with the remark, but I think the benchmark documentation at https://github.com/maplibre/maplibre-gl-js/blob/main/test/bench/README.md still needs to be updated to match this changes.

HarelM added 2 commits February 10, 2022 17:18

Move bench under test folder

9671ba7

Add missing test work in readme

0793ef9

HarelM requested review from wipfli and birkskyum February 10, 2022 15:33

birkskyum reviewed Feb 10, 2022

View reviewed changes

test/bench/run-benchmarks.ts Show resolved Hide resolved

HarelM added 2 commits February 10, 2022 17:40

Update run-benchmarks.ts

aa655be

Update run-benchmarks.ts

4b19df2

birkskyum requested changes Feb 10, 2022

View reviewed changes

Update upload-benchmarks.yml

ff2df96

birkskyum approved these changes Feb 10, 2022

View reviewed changes

wipfli reviewed Feb 10, 2022

View reviewed changes

test/bench/README.md Outdated Show resolved Hide resolved

wipfli reviewed Feb 10, 2022

View reviewed changes

test/bench/README.md Outdated Show resolved Hide resolved

birkskyum requested changes Feb 10, 2022

View reviewed changes

HarelM mentioned this pull request Feb 10, 2022

Redesign how benchmarks work #982

Open

starting server at test folder

99107f3

HarelM added 4 commits February 11, 2022 09:40

revert readme changes

50cd141

revert changes in styles index html

d5b0b17

Add console log when starting the run

a909e50

Merge branch 'main' into move-bench

f3699cb

removed css references

c19e0b7

birkskyum approved these changes Feb 11, 2022

View reviewed changes

birkskyum merged commit 8ff0907 into main Feb 11, 2022

birkskyum deleted the move-bench branch February 11, 2022 09:53

wipfli mentioned this pull request Mar 9, 2022

Fix benchmark paths #1079

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move bench under `test` folder #979

Move bench under `test` folder #979

HarelM commented Feb 10, 2022

github-actions bot commented Feb 10, 2022 •

edited

Loading

birkskyum commented Feb 10, 2022 •

edited

Loading

birkskyum left a comment •

edited

Loading

HarelM commented Feb 10, 2022

wipfli commented Feb 10, 2022

wipfli commented Feb 10, 2022

birkskyum left a comment •

edited

Loading

wipfli commented Feb 10, 2022

HarelM commented Feb 10, 2022

birkskyum commented Feb 10, 2022 •

edited

Loading

birkskyum commented Feb 10, 2022

HarelM commented Feb 10, 2022

HarelM commented Feb 11, 2022

birkskyum commented Feb 11, 2022 •

edited

Loading

HarelM commented Feb 11, 2022

birkskyum commented Feb 11, 2022

HarelM commented Feb 11, 2022

birkskyum commented Feb 11, 2022

birkskyum left a comment

xabbu42 commented Feb 15, 2022

xabbu42 commented Mar 10, 2022

Move bench under test folder #979

Move bench under test folder #979

Conversation

HarelM commented Feb 10, 2022

Launch Checklist

github-actions bot commented Feb 10, 2022 • edited Loading

birkskyum commented Feb 10, 2022 • edited Loading

birkskyum left a comment • edited Loading

Choose a reason for hiding this comment

HarelM commented Feb 10, 2022

wipfli commented Feb 10, 2022

wipfli commented Feb 10, 2022

birkskyum left a comment • edited Loading

Choose a reason for hiding this comment

wipfli commented Feb 10, 2022

HarelM commented Feb 10, 2022

birkskyum commented Feb 10, 2022 • edited Loading

birkskyum commented Feb 10, 2022

HarelM commented Feb 10, 2022

HarelM commented Feb 11, 2022

birkskyum commented Feb 11, 2022 • edited Loading

HarelM commented Feb 11, 2022

birkskyum commented Feb 11, 2022

HarelM commented Feb 11, 2022

birkskyum commented Feb 11, 2022

birkskyum left a comment

Choose a reason for hiding this comment

xabbu42 commented Feb 15, 2022

xabbu42 commented Mar 10, 2022

Move bench under `test` folder #979

Move bench under `test` folder #979

github-actions bot commented Feb 10, 2022 •

edited

Loading

birkskyum commented Feb 10, 2022 •

edited

Loading

birkskyum left a comment •

edited

Loading

birkskyum left a comment •

edited

Loading

birkskyum commented Feb 10, 2022 •

edited

Loading

birkskyum commented Feb 11, 2022 •

edited

Loading