Merged
Conversation
drewdaemon
added
Feature:ES|QL
Contributor
|
Pinging @elastic/kibana-esql (Team:ESQL) |
Contributor
Author
|
@elasticmachine merge upstream |
Contributor
|
I will wait for the CI to get green and review afterwards |
Contributor
|
@drewdaemon I see 2 tasks in the same PR:
They are not connected as far as I can understand, are they? I wonder if it makes sense to break it into 2 PRs as is a bit difficult to me to understand which are the things necessary to change from the worker removal (also in case of revert we will revert only the worker changes) |
Contributor
|
/ci |
Contributor
Author
|
@stratoula point taken—they are connected but not strongly—I should probably not have gone as far as I did with the related cleanup.
You can see just the worker changes in 0a4713e Hopefully that helps... let me know if you still want me to put in the effort to pull these tasks apart. |
stratoula
approved these changes
Apr 15, 2025
Contributor
stratoula
left a comment
stratoula
left a comment
There was a problem hiding this comment.
The commit helped Drew, thanx!
LGTM, I tested it locally and I can't see any performance degradation. (also much simpler code)
Contributor
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
References to deprecated APIs
Total ESLint disabled count
History
|
patrykkopycinski
approved these changes
Apr 15, 2025
Dosant
approved these changes
Apr 15, 2025
viduni94
approved these changes
Apr 15, 2025
darnautov
approved these changes
Apr 15, 2025
Comment on lines
+11
to
+12
| import { camelCase } from 'lodash'; | ||
| import capitalize from 'lodash/capitalize'; |
Contributor
There was a problem hiding this comment.
nit: I understand that IDE just reorganized imports
Suggested change
| import { camelCase } from 'lodash'; | |
| import capitalize from 'lodash/capitalize'; | |
| import { camelCase, capitalize } from 'lodash'; |
Contributor
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/14474364386 |
Contributor
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
Contributor
Author
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
drewdaemon
added a commit
to drewdaemon/kibana
that referenced
this pull request
Apr 15, 2025
## Summary Fix elastic#217923 Investigations in elastic#217368 showed that there was basically no performance impact to passing the AST across a thread boundary. But we also didn't detect a pressing reason to remove the worker. Since then, however, we noticed another cost associated with the worker: it's a hefty Javascript file, even in production builds. In addition, we are doing parsing on the main thread _and_ the worker, so the `kbn-esql-ast` package is actually being loaded and parsed twice by the browser, once for the main thread and once for the worker. This PR removes our worker. Our parsing associated with validation and autocomplete will still be done asynchronously, but on the main thread. I do not see any regression in perceived performance. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> (cherry picked from commit 9b4403b) # Conflicts: # src/platform/packages/shared/kbn-monaco/src/esql/language.ts # src/platform/packages/shared/kbn-monaco/src/esql/lib/converters/suggestions.ts # src/platform/packages/shared/kbn-monaco/src/esql/lib/esql_ast_provider.ts # src/platform/packages/shared/kbn-monaco/src/esql/lib/signature/index.ts # src/platform/packages/shared/kbn-monaco/src/esql/lib/types.ts # src/platform/packages/shared/kbn-monaco/src/esql/worker/esql.worker.ts # src/platform/packages/shared/kbn-monaco/src/esql/worker/esql_worker.ts # src/platform/packages/shared/kbn-monaco/src/register_globals.ts # src/platform/packages/shared/kbn-monaco/webpack.config.js
1 task
drewdaemon
added a commit
that referenced
this pull request
Apr 16, 2025
# Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] remove worker (#218006)](#218006) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Drew Tate","email":"drew.tate@elastic.co"},"sourceCommit":{"committedDate":"2025-04-15T16:18:07Z","message":"[ES|QL] remove worker (#218006)\n\n## Summary\n\nFix https://github.com/elastic/kibana/issues/217923\n\nInvestigations in #217368 showed\nthat there was basically no performance impact to passing the AST across\na thread boundary. But we also didn't detect a pressing reason to remove\nthe worker.\n\nSince then, however, we noticed another cost associated with the worker:\nit's a hefty Javascript file, even in production builds. In addition, we\nare doing parsing on the main thread _and_ the worker, so the\n`kbn-esql-ast` package is actually being loaded and parsed twice by the\nbrowser, once for the main thread and once for the worker.\n\nThis PR removes our worker. Our parsing associated with validation and\nautocomplete will still be done asynchronously, but on the main thread.\n\nI do not see any regression in perceived performance.\n\n---------\n\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>\nCo-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>","sha":"9b4403b7dc867de1db520e1f9e8ed07f22f41784","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Feature:ES|QL","Team:ESQL","backport:version","v9.1.0","v8.19.0"],"title":"[ES|QL] remove worker","number":218006,"url":"https://github.com/elastic/kibana/pull/218006","mergeCommit":{"message":"[ES|QL] remove worker (#218006)\n\n## Summary\n\nFix https://github.com/elastic/kibana/issues/217923\n\nInvestigations in #217368 showed\nthat there was basically no performance impact to passing the AST across\na thread boundary. But we also didn't detect a pressing reason to remove\nthe worker.\n\nSince then, however, we noticed another cost associated with the worker:\nit's a hefty Javascript file, even in production builds. In addition, we\nare doing parsing on the main thread _and_ the worker, so the\n`kbn-esql-ast` package is actually being loaded and parsed twice by the\nbrowser, once for the main thread and once for the worker.\n\nThis PR removes our worker. Our parsing associated with validation and\nautocomplete will still be done asynchronously, but on the main thread.\n\nI do not see any regression in perceived performance.\n\n---------\n\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>\nCo-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>","sha":"9b4403b7dc867de1db520e1f9e8ed07f22f41784"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/218006","number":218006,"mergeCommit":{"message":"[ES|QL] remove worker (#218006)\n\n## Summary\n\nFix https://github.com/elastic/kibana/issues/217923\n\nInvestigations in #217368 showed\nthat there was basically no performance impact to passing the AST across\na thread boundary. But we also didn't detect a pressing reason to remove\nthe worker.\n\nSince then, however, we noticed another cost associated with the worker:\nit's a hefty Javascript file, even in production builds. In addition, we\nare doing parsing on the main thread _and_ the worker, so the\n`kbn-esql-ast` package is actually being loaded and parsed twice by the\nbrowser, once for the main thread and once for the worker.\n\nThis PR removes our worker. Our parsing associated with validation and\nautocomplete will still be done asynchronously, but on the main thread.\n\nI do not see any regression in perceived performance.\n\n---------\n\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>\nCo-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>","sha":"9b4403b7dc867de1db520e1f9e8ed07f22f41784"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
drewdaemon
added a commit
to drewdaemon/kibana
that referenced
this pull request
Apr 30, 2025
Fix elastic#217923 Investigations in elastic#217368 showed that there was basically no performance impact to passing the AST across a thread boundary. But we also didn't detect a pressing reason to remove the worker. Since then, however, we noticed another cost associated with the worker: it's a hefty Javascript file, even in production builds. In addition, we are doing parsing on the main thread _and_ the worker, so the `kbn-esql-ast` package is actually being loaded and parsed twice by the browser, once for the main thread and once for the worker. This PR removes our worker. Our parsing associated with validation and autocomplete will still be done asynchronously, but on the main thread. I do not see any regression in perceived performance. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> (cherry picked from commit 9b4403b)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix #217923
Investigations in #217368 showed that there was basically no performance impact to passing the AST across a thread boundary. But we also didn't detect a pressing reason to remove the worker.
Since then, however, we noticed another cost associated with the worker: it's a hefty Javascript file, even in production builds. In addition, we are doing parsing on the main thread and the worker, so the
kbn-esql-astpackage is actually being loaded and parsed twice by the browser, once for the main thread and once for the worker.This PR removes our worker. Our parsing associated with validation and autocomplete will still be done asynchronously, but on the main thread.
I do not see any regression in perceived performance.