From an strace it can take up to 6 sec on our servers. Ex: First stat: 07:48:46.941069 (+ 0.000129) brk(0x5641224ad000) = 0x5641224ad000 <0.000018> 07:48:46.941340 (+ 0.000015) stat("/home/koha/src/api/v1/swagger/swagger.yaml", {st_mode=S_IFREG|0644, st_size=30696, ...}) = 0 <0.000010> ... last one 07:48:53.453980 (+ 0.000030) lstat("/home/koha/src/api/v1/swagger/swagger.yaml", {st_mode=S_IFREG|0644, st_size=30696, ...}) = 0 <0.000015>
Created attachment 164520 [details] Flamegraph of debian/templates/plack.psgi I also noticed that startup time was really long and ran debian/templates/plack.psgi with Devel::NYTProf to see what was happening. And indeed the YAML parsing/validation is what takes the most time (95% of the startup is spent into Koha::REST::V1::startup Here's the flamegraph to give an idea of the problem
Didier and I made some testing and bundling the spec reduces the startup time significantly (5x).
Created attachment 165412 [details] [review] Bug 36546: Add a bundled version of the API spec This patch makes the Koha::REST::V1 class use a bundled version of the API spec. For that to happen, this patch also adds: * a `yarn api:bundle` task for generating the `swagger_bundle.yaml` file * the bundle file to `.gitignore` * @redocly/cli to the packages.json dependencies (yarn add @redocly/cli) * `api:bundle` is added to the `yarn build` general command NOTE: A new workflow is introduced by this patch as developers will need to call the `yarn api:bundle` command on each API spec change they make. To test: 1. Have KTD running 2. Verify the API works as usual => SUCCESS: The API just works 3. Apply this patch 4. Run: $ ktd --shell k$ yarn api:bundle 5. Restart plack: k$ koha-plack --restart kohadev ; tail -f /var/log/koha/kohadev/*.log => SUCCESS: The API loads correctly 6. Repeat 2 => SUCCESS: Nothing changed 7. Sign off :-D
Putting this here for review/discussion. Feedback is welcome!
I am so happy to see other people talking about this, because this has been driving me crazy for *years*. It's arguably my biggest pet peeve about Koha! A few years ago, I got Jan Henning Thorsen to add "skip_validating_specification" to Mojolicious::Plugin::OpenAPI, and I've turned that on locally in Koha, but it hasn't made a noticeable difference to startup times, and when I use strace I notice something similar to Didier. The same syscalls over and over again: newfstatat(AT_FDCWD, "/usr", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(AT_FDCWD, "/usr/share", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(AT_FDCWD, "/usr/share/koha", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(AT_FDCWD, "/usr/share/koha/api", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(AT_FDCWD, "/usr/share/koha/api/v1", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(AT_FDCWD, "/usr/share/koha/api/v1/swagger", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(AT_FDCWD, "/usr/share/koha/api/v1/swagger/swagger.yaml", {st_mode=S_IFREG|0644, st_size=44125, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(AT_FDCWD, "/usr/share/koha/api/v1/swagger/swagger.yaml", {st_mode=S_IFREG|0644, st_size=44125, ...}, 0) = 0 Jan was always skeptical about the validation being the problem, but he humoured me nonetheless. In my tests in 2021, skipping validation solved the problem, but using "skip_validating_specification" hasn't helped in 2024 (and earlier years). Even back then, I figured the problem was with the dereferencing of the spec, but it seemed to only happen with the validation. I'd be interested to see a flamegraph generated with Tomas's patch. I'm going to give it a go now and see what strace says...
(In reply to Tomás Cohen Arazi from comment #4) > Putting this here for review/discussion. > > Feedback is welcome! Unfortunately, I couldn't get this patch to work. I did a "yarn install" then "yarn api:bundle" and this is what I got: yarn run v1.22.19 $ redocly bundle api/v1/swagger/swagger.yaml > api/v1/swagger/swagger_bundle.yaml /kohadevbox/koha/node_modules/@redocly/openapi-core/lib/js-yaml/index.js:9 implicit: [js_yaml_1.types.merge], ^ TypeError: Cannot read properties of undefined (reading 'merge') at Object.<anonymous> (/kohadevbox/koha/node_modules/@redocly/openapi-core/lib/js-yaml/index.js:9:32) at Module._compile (node:internal/modules/cjs/loader:1256:14) at Module._extensions..js (node:internal/modules/cjs/loader:1310:10) at Module.load (node:internal/modules/cjs/loader:1119:32) at Module._load (node:internal/modules/cjs/loader:960:12) at Module.require (node:internal/modules/cjs/loader:1143:19) at require (node:internal/modules/cjs/helpers:121:18) at Object.<anonymous> (/kohadevbox/koha/node_modules/@redocly/openapi-core/lib/utils.js:18:19) at Module._compile (node:internal/modules/cjs/loader:1256:14) at Module._extensions..js (node:internal/modules/cjs/loader:1310:10) Node.js v18.17.1 error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
If I take down Koha and try to start it up, it breaks koha-testing-docker as it fails on the yarn api:bundle and exits.
rm -rf node_modules/ yarn install Still doesn't work. Trying to troubleshoot the issue, but it looks like it should be working when I look through the JS files.
(In reply to David Cook from comment #8) > rm -rf node_modules/ > yarn install > > Still doesn't work. > > Trying to troubleshoot the issue, but it looks like it should be working > when I look through the JS files. It is weird, I didn’t manage to make it run before I left the office. If you want to try it, run it through npx instead
(In reply to David Cook from comment #8) > Trying to troubleshoot the issue, but it looks like it should be working > when I look through the JS files. I mean the js-yaml version does seem incompatible. It should be 4.1.0 instead of 3.13.1, but even after upgrading it I still get the same error... -- rm -rf node_modules rm yarn.lock yarn install No luck. Node.js/yarn are so weird... -- cd /kohadevbox/koha/node_modules/@redocly/openapi-core yarn install This installs @redocly's openapi-core dependencies into /kohadevbox/koha/node_modules/@redocly/openapi-core/node_modules, which includes js-yaml 4.1.0 while kohadevbox/koha/node_modules/js-yaml is still 3.13.1 -- Now I can get redocly to run, but it looks like this isn't going to work out of the box...
(In reply to David Cook from comment #10) > Now I can get redocly to run, but it looks like this isn't going to work out > of the box... kohadev-koha@kohadevbox:koha(bug36546)$ redocly bundle api/v1/swagger/swagger.yaml > api/v1/swagger/swagger_bundle.yaml bundling api/v1/swagger/swagger.yaml... � Created a bundle for api/v1/swagger/swagger.yaml at stdout 422ms.
Btw, I think the problem is that "js-yaml" is in "resolutions" in our package.json. That's overriding the dependency for @redocly/openapi-core it seems. Looks like Owen did that back in 2019 for bug 23025 Something to revisit it seems.
(In reply to David Cook from comment #12) > Btw, I think the problem is that "js-yaml" is in "resolutions" in our > package.json. > > That's overriding the dependency for @redocly/openapi-core it seems. > > Looks like Owen did that back in 2019 for bug 23025 > > Something to revisit it seems. find node_modules -name "package.json" -exec grep 'js-yaml' {} \; "name": "@types/js-yaml", "description": "TypeScript definitions for js-yaml", "homepage": "https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/js-yaml", "directory": "types/js-yaml" "name": "js-yaml", "repository": "nodeca/js-yaml", "js-yaml": "bin/js-yaml.js" "module": "./dist/js-yaml.mjs", "import": "./dist/js-yaml.mjs", "unpkg": "dist/js-yaml.min.js", "jsdelivr": "dist/js-yaml.min.js", "js-yaml": "^4.1.0", "@types/js-yaml": "^4.0.3", "js-yaml": "^4.1.0", "name": "js-yaml", "homepage": "https://github.com/nodeca/js-yaml", "repository": "nodeca/js-yaml", "js-yaml": "bin/js-yaml.js" "unpkg": "dist/js-yaml.min.js", "jsdelivr": "dist/js-yaml.min.js", Probably time to upgrade our js-yaml version and stop the "resolutions" override...
Comment on attachment 165412 [details] [review] Bug 36546: Add a bundled version of the API spec Review of attachment 165412 [details] [review]: ----------------------------------------------------------------- ::: Koha/REST/V1.pm @@ +75,4 @@ > $self->secrets([$secret_passphrase]); > } > > + my $spec_file = $self->home->rel_file("api/v1/swagger/swagger_bundle.yaml"); Perhaps we should test for "swagger_bundle.yaml" and fallback to "swagger.yaml" if it's unavailable?
Created attachment 165446 [details] [review] Bug 36546: Fix js-yaml dependency in package.json
I think perhaps we should use swagger.yml for development, and then we could build swagger_bundle.yml during the build. In koha-testing-docker, Koha is starting so fast I can't catch the Swagger parsing with strace. I'm excited to try this out on a test server with multiple instances and more realistic load than my dev machine. Great work here, Tomas (and everyone else)!
Wow, just tested this. Awesome!
(In reply to David Cook from comment #16) > I think perhaps we should use swagger.yml for development, and then we could > build swagger_bundle.yml during the build. I would say: use swagger_bundle.yml if it exists, fallback to swagger.yml if not. A slow startup is better than a failed one :) And using swagger_bundle.yml while developing something unrelated to the API would also improve the code/restart/test cycle speed
Created attachment 165449 [details] [review] Bug 36546: Add a bundled version of the API spec This patch makes the Koha::REST::V1 class use a bundled version of the API spec. For that to happen, this patch also adds: * a `yarn api:bundle` task for generating the `swagger_bundle.yaml` file * the bundle file to `.gitignore` * @redocly/cli to the packages.json dependencies (yarn add @redocly/cli) * `api:bundle` is added to the `yarn build` general command NOTE: A new workflow is introduced by this patch as developers will need to call the `yarn api:bundle` command on each API spec change they make. To test: 1. Have KTD running 2. Verify the API works as usual => SUCCESS: The API just works 3. Apply this patch 4. Run: $ ktd --shell k$ yarn api:bundle 5. Restart plack: k$ koha-plack --restart kohadev ; tail -f /var/log/koha/kohadev/*.log => SUCCESS: The API loads correctly 6. Repeat 2 => SUCCESS: Nothing changed 7. Sign off :-D Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 165450 [details] [review] Bug 36546: Fix js-yaml dependency in package.json Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Still leaving this in discussion for additional opinions but the approach is sane, the gain is measurable and all tests pass (which is to be expected, but still..). Would love for this to move forward.
(In reply to Tomás Cohen Arazi from comment #2) > Didier and I made some testing and bundling the spec reduces the startup > time significantly (5x). I see an improvement but not that significant. I ran the following command: time KOHA_HOME=$(pwd) perl debian/templates/plack.psgi On master, the average time is 3.5s With the patch, it's 2.4s The new flamegraph is not that much different, except that the `resolve` call takes less time relative to others Still a nice improvement!
Would be interesting how JSON would perform compared to yaml as the spec is much simpler. Maybe worth a try?
Created attachment 165550 [details] [review] [OPTIONAL] Bug 36546: Use JSON instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Just tested the JSON patch as well. There is another small gain: (Used the same thing as Julian: `time KOHA_HOME=$(pwd) perl debian/templates/plack.psgi`) Without the patches applied: real 0m4.225s user 0m4.117s sys 0m0.101s With the first 2 applied: real 0m3.065s user 0m2.983s sys 0m0.074s With the JSON patch (3rd) on top: real 0m2.704s user 0m2.611s sys 0m0.086s I think it wouldn't hurt to use JSON for the bundle.
Created attachment 165581 [details] [review] Bug 36546: Use JSON instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 165589 [details] [review] Bug 36546: Use JSON instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Failing this for now, since it's not deployable. 1. We need package.json to include yarn api:bundle in build:prod. 2. swagger_bundle.json will need to be added to the file_map in Makefile.PL as produced by build-resources.PL I think we should also include a fallback to swagger.yml if swagger_bundled.json is missing, although technically it should be installed by "make install", so I suppose it's not the end of the world... I can look at writing that missing patch...
Created attachment 165821 [details] [review] Bug 36546: Deploy swagger_bundle.json via make To test: 0. Apply patch 1a. perl Makefile.PL 1b. Choose "single" Installation mode 1c. Specify /opt/koha directory 2. make 2b. ls blib/API_CGI_DIR/v1/swagger/swagger_bundle.json 3. Ensure you're running as root or make koha user own /opt 4. make install 5. ls /opt/koha/api/v1/swagger/swagger_bundle.json
Actually yeah I think we do need the failover to swagger.json or else many users of koha-testing-docker will have broken Koha instances...
(In reply to David Cook from comment #30) > Actually yeah I think we do need the failover to swagger.json or else many > users of koha-testing-docker will have broken Koha instances... Ah wait... my bad. ktd runs yarn build so you should have a api/v1/swagger/swagger_bundle.json Then any core API changes will require a rebuild. Not the end of the world...
Created attachment 165828 [details] [review] Bug 36546: Add a bundled version of the API spec This patch makes the Koha::REST::V1 class use a bundled version of the API spec. For that to happen, this patch also adds: * a `yarn api:bundle` task for generating the `swagger_bundle.yaml` file * the bundle file to `.gitignore` * @redocly/cli to the packages.json dependencies (yarn add @redocly/cli) * `api:bundle` is added to the `yarn build` general command NOTE: A new workflow is introduced by this patch as developers will need to call the `yarn api:bundle` command on each API spec change they make. To test: 1. Have KTD running 2. Verify the API works as usual => SUCCESS: The API just works 3. Apply this patch 4. Run: $ ktd --shell k$ yarn api:bundle 5. Restart plack: k$ koha-plack --restart kohadev ; tail -f /var/log/koha/kohadev/*.log => SUCCESS: The API loads correctly 6. Repeat 2 => SUCCESS: Nothing changed 7. Sign off :-D Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165829 [details] [review] Bug 36546: Fix js-yaml dependency in package.json Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165830 [details] [review] Bug 36546: Use JSON instead Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165831 [details] [review] Bug 36546: Deploy swagger_bundle.json via make To test: 0. Apply patch 1a. perl Makefile.PL 1b. Choose "single" Installation mode 1c. Specify /opt/koha directory 2. make 2b. ls blib/API_CGI_DIR/v1/swagger/swagger_bundle.json 3. Ensure you're running as root or make koha user own /opt 4. make install 5. ls /opt/koha/api/v1/swagger/swagger_bundle.json Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This shows a pretty solid improvement for me here too.. I think the polishing that's gone in between the authors is great and I'm happy to QA. I've added the Signoff lines from each of the testing parties and am going to take their comments and refinements as signoffs respectively. QA scripts are happy, KTD starts up nicely with the patches applied (and even highlighted a bug that had been missed in main). Passing QA
Pushed for 24.05! Well done everyone, thank you!
I suspect this to have broken API driven things like patron search: Something went wrong when loading the table. 404: Not Found. HELP.
When I run yarn:build locally it first complains about an outdated module and then leaves me with an untracked file: api/v1/swagger/swagger_bundle.json
Created attachment 165923 [details] [review] Bug 36546: (QA follow-up) Add bundle spec to .gitignore Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Katrin Fischer from comment #38) > I suspect this to have broken API driven things like patron search: > > Something went wrong when loading the table. > 404: Not Found. > > HELP. This is related to bug 36722. I was able to reproduce the failure, but at some point it started working again. So probably a networking issue with json-schema.org The right solution is on 36722. And should be done on packaging.
(In reply to Katrin Fischer from comment #39) > When I run yarn:build locally it first complains about an outdated module It 'lets you know' there's a newer version of the tool we use to bundle the spec. Not an error per-se. > and then leaves me with an untracked file: > api/v1/swagger/swagger_bundle.json That's the expected output for the command. It highlights the 'Use JSON instead' patch missed to change the file name on the .gitignore file. Follow-up submitted for fixing it.
(In reply to David Cook from comment #28) > Failing this for now, since it's not deployable. > > 1. We need package.json to include yarn api:bundle in build:prod. I'm having hard time understanding how we lost that line, which was included on the first patch! I don't see any patch removing it by itself!
Darn, I should have caught the gitignore regression during QA, apologies
(In reply to Katrin Fischer from comment #39) > When I run yarn:build locally it first complains about an outdated module > and then leaves me with an untracked file: > > api/v1/swagger/swagger_bundle.json You'll need to "rm -rf node_modules" and re-run "yarn install" to fix the outdated module issue.
(In reply to Tomás Cohen Arazi from comment #43) > (In reply to David Cook from comment #28) > > Failing this for now, since it's not deployable. > > > > 1. We need package.json to include yarn api:bundle in build:prod. > > I'm having hard time understanding how we lost that line, which was included > on the first patch! I don't see any patch removing it by itself! The first patch added it to "build" but not to "build:prod". So nothing was lost. Just missed!
(At some point, it could be interesting to look at resolving the API spec in a module which could be pre-loaded like in bug 36721, because that could deliver even further performance gains.)
Created attachment 165945 [details] [review] Bug 36546: (follow-up) Add fallback to unbundled spec Some developers are running without ktd still and as such may not want to install the node modules required to bundle the specification. This patch adds a fallback to the unbundled yaml files when the bundle.json file isn't found. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Remaining follow-up pushed to main: Bug 36546: (follow-up) Add fallback to unbundled spec
What are we thinking in terms of backporting this one?
(In reply to David Cook from comment #51) > What are we thinking in terms of backporting this one? I'm on it. Looks like a change that may affect all API. Some non-packaged, non-debian install may be surprised about this change. For me this is not a change we should apply on stable branches.
(In reply to Fridolin Somers from comment #52) > (In reply to David Cook from comment #51) > > What are we thinking in terms of backporting this one? > > I'm on it. > Looks like a change that may affect all API. > Some non-packaged, non-debian install may be surprised about this change. > For me this is not a change we should apply on stable branches. In theory, non-packaged non-debian installs should be OK so long as they update using "make". Even for a git/dev install that doesn't, the fallback to the unbundled spec should still work. Let me know if I can help in any way :)