-
Notifications
You must be signed in to change notification settings - Fork 1
[W-20742893] refactor: make cross-file reference resolution on demand #183
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
Conversation
…based orchestration - Convert precompile, compile, lint, bundle, and package tasks from command-based to dependency-based approach - Remove hardcoded package-specific paths from root config - Add explicit cross-package dependency for copy-worker-files on apex-ls:bundle - Follows VSE monorepo pattern for better maintainability and caching
- Add ../apex-parser-ast:compile dependency to ensure parser-ast is compiled before apex-ls - Add ../lsp-compliant-services:compile dependency to ensure proper build order for TypeScript project references - Fixes CI/CD linting failure where apex-ls was compiling before its dependencies
- Change test to check for path.join pattern instead of literal 'dist/resources' string - Test now correctly validates that resourcesDir is constructed using path.join - Fixes CI test failure where test expected literal string match
All package tsconfig.json files extend the root tsconfig.base.json, so Wireit compile tasks should track changes to the base config to ensure proper cache invalidation and rebuilds when the root TypeScript config changes.
- Replace manual file copying functions with esbuild-plugin-copy plugin - Update apex-ls, apex-parser-ast, and apex-lsp-vscode-extension esbuild configs - Update tests to verify plugin configuration instead of manual copy functions - Add esbuild-plugin-copy dependency to affected packages
… hover Fix missing artifact resolution for hover requests when hovering over chained references like FileUtilities.createFile. Previously, the code would extract only the method name (createFile) instead of the full chain (FileUtilities.createFile), causing incorrect search behavior. Changes: - Updated extractReferenceAtPosition to prioritize chained references over individual references when both exist at the same position - Updated generateSearchHints to extract qualifier from chained references' chainNodes - Updated inferQualifierType to treat chained references as class references - Updated resolveQualifierInfo to extract qualifier from chainNodes - Added test cases to verify chained references are prioritized correctly
- Add missing entry points to knip config (index.ts files) - Set ignoreExportsUsedInFile to false for better unused export detection - Remove unused dev dependencies (@jest/test-sequencer, @salesforce/apex-lsp-parser-ast) - Add missing dependencies (effect, @salesforce/apex-lsp-parser-ast, @salesforce/apex-lsp-compliant-services) - Remove redundant entry patterns and non-existent file references - Remove wait from ignoreBinaries and build-config entry pattern
… for Windows compatibility
…st directory error
…queue state callbacks
Move scheduler initialization earlier in LCSAdapter startup to ensure it's ready when didOpen events arrive. Add fast path check in LSPQueueManager to avoid redundant initialization checks.
- Add queueStateNotificationIntervalMs config setting (default: 200ms) - Remove callback invocations from processQueuedItem and controllerLoop - Create startQueueStateNotificationTask() for periodic notifications - Update LCSAdapter to start periodic task instead of registering callback - Notifications now sent at configurable intervals independent of queue processing - Simplifies code by removing throttling and race condition logic
- Add workspace state querying to references service - Enhance queue management for workspace loads - Improve hover and references processing services - Add server configuration enhancements - Update dependencies
- Reduce queue processing concurrency limits to prevent resource exhaustion - Add yield budget to scheduler loop to prevent event loop blocking - Replace microtask yields with macrotask yields for better I/O responsiveness - Simplify workspace load coordination using notifications instead of queries - Remove excessive debug logging from queue state notification task - Update default settings for better performance under load
- Add batch workspace loading mechanism using compressed ZIP files - Implement client-side batch creation, compression, and sequential sending - Add server-side batch handler for decompression and didOpen enqueueing - Add batchSize configuration option for workspace loading - Use fflate for compression/decompression with fixed level 6 - Process batches sequentially to prevent queue overflow
- Add addSymbolTableMinimal() method that skips reference processing during file open - Add resolveCrossFileReferencesForFile() for on-demand cross-file ref resolution - Update DocumentProcessingService to use minimal symbol table addition - Update DocumentSaveProcessingService to use minimal symbol table addition - Update DiagnosticProcessingService to resolve cross-file refs on-demand before diagnostics - Add resolveCrossFileReferencesForFile() to ISymbolManager interface This change prevents task explosion during workspace loading by deferring cross-file reference resolution until needed (hover, goto definition, diagnostics). Workspace loading is now faster with lower queue pressure, while maintaining full functionality through on-demand resolution.
…bose log - Add Workspace Loading section to performance settings UI - Include batchSize, maxConcurrency, yieldInterval, yieldDelayMs, and enabled settings - Remove verbose 'Queue metrics changed, calling callback' log entry
- Add comprehensive test verifying cross-file references are resolved and added to graph after resolveCrossFileReferencesForFile - Fix lint errors: remove unused imports and variables, fix formatting - Update test expectations to reflect separation of same-file and cross-file resolution
Same-file references that cannot be resolved during second-pass resolution should be skipped rather than deferred, as deferring won't help for same-file references (the target is in the same file and should have been found during second-pass). This change applies to both: - processSameFileReferenceToGraphEffect (used during addSymbolTable) - processSymbolReferenceToGraphEffect (used during on-demand resolution) Only cross-file references should be deferred, as they may be resolved when the target file is loaded. This reduces unnecessary deferred reference processing during workspace loading and improves UI responsiveness.
…integration analysis
…e status display - Remove automatic queueing of deferred references in ApexSymbolGraph.addSymbol() Deferred references should only be processed on-demand, not automatically during workspace load to maintain lazy behavior - Simplify queue status display to show queued/active/complete format instead of separate sections for each state
- Add EDA workspace performance test suite - Add EDA performance helpers for repository cloning - Fix benchmark suite cleanup and timeout handling - Improve child process security by using spawn instead of exec - Add test filtering for specific performance test suites - Add QUICK mode for faster validation runs feat: add layered symbol collection and progressive enhancement - Add FullSymbolCollectorListener wrapper combining layered listeners - Add ListenerFactory for creating appropriate listeners per service - Add ProgressiveEnhancementService for demand-driven symbol enrichment - Add trigger support and createNewInstance to all layered listeners - Update LSP services to use PublicAPISymbolListener for better performance - Update tests to use FullSymbolCollectorListener where appropriate - Export new components from package index This enables progressive enhancement of symbol tables from least to most expensive operations, improving LSP startup performance while maintaining full symbol collection capabilities when needed. feat(parser): add layered symbol listeners and separate reference collection/resolution - Add PublicAPISymbolListener, ProtectedSymbolListener, and PrivateSymbolListener for progressive symbol collection - Add ApexReferenceCollectorListener for dedicated reference capture - Add ApexReferenceResolver for separated reference resolution - Refactor ApexSymbolCollectorListener to delegate reference resolution to ApexReferenceResolver - Add compileLayered() method to CompilerService for multi-pass compilation - Add ListenerApplicationManager for managing listener dependencies - Add SymbolTable enrichment support via _detailLevel property - Update DocumentStateCache to track detail levels and parse trees - Add comprehensive tests for layered listeners and reference separation - Update performance tests to benchmark layered listener approach - Update README with new architecture documentation
| async (params: LoadWorkspaceParams): Promise<LoadWorkspaceResult> => { | ||
| // Register workspace load completion notification handlers | ||
| this.connection.onNotification( | ||
| 'apex/workspaceLoadComplete', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this PR but one day, I think moving registration of custom registered endpoints in our server vs the standard ones, into their own classes for organization and to reduce the size of LCSAdapter might be a good idea. The class itself is getting pretty big and I think it might be easier to parse down the road that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logged W-20804806
| /** | ||
| * Decode base64 string to Uint8Array | ||
| */ | ||
| function decodeBase64(base64: string): Uint8Array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to include the Buffer version at all if the Browser env version of decoding is viable for both? That way we could remove the Buffer reference entirely from the file
| } | ||
|
|
||
| const yieldToEventLoop = Effect.async<void>((resume) => { | ||
| setImmediate(() => resume(Effect.void)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain what this is doing? A chained empty promise that has an immediate timeout to return to void?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that a simple Effect.yieldNow or Effect.sleep(0) was not really causing the scheduler to yield to the event loop. The use of set immediate enques a V8 macrotask that is handled independently, at the top of the event loop, where as yieldNow and sleep are dealt with later.
Guaranteess that the scheduler will give the event loop a chance to run.
kylewalke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions but nothing blocking.
- Add lint:fix wireit configuration to root and all workspace packages - Fix line length violations in DiagnosticProcessingService - Fix unused variables and catch error parameters across codebase - Update npm packageManager to 11.7.0 - Replace workspace:* protocol with version numbers for npm compatibility
- Remove packageManager field requiring [email protected] - Update engines.npm from >=11.0.0 to >=10.0.0 to match Node.js 20 LTS bundled npm
- Add globalTeardown to root jest.config.cjs to ensure cleanup runs - Add timeouts to jest-teardown.js to prevent hanging on shutdown operations - Add 30-minute timeout to CI test job to catch any remaining hangs - Prevents scheduler shutdown operations from blocking test completion in CI
…dotcom/apex-language-support into phale/more-tweaks-for-refs
- Use path.resolve with __dirname to get absolute path to teardown script - Fixes issue where packages extending base config couldn't find teardown script - Ensures globalTeardown works correctly whether run from root or package directories
…meouts - Remove .perf.ts files from testMatch in apex-lsp-testbed jest config - Performance tests should only run via test:perf command, not during unit tests - Increase performance test timeouts for CI environments (10min for batch, 5min for incremental) - Prevents performance tests from causing timeouts in CI unit test runs
- Increase ResourceLoaderIntegration beforeAll timeout to 5min for CI, 3min for local - Increase PriorityScheduler idle sleep test upper bound to 150ms for CI - Accounts for slower CI environments while maintaining test validity
- Increase timeout to 5min for CI, 3min for local - Fixes timeout failures on Windows CI where full compilation takes longer - Matches timeout pattern used in ResourceLoaderIntegration tests
| ] | ||
| }, | ||
| "lint:fix": { | ||
| "dependencies": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
- Update 12 snapshots for textDocument/documentSymbol requests - Reflects changes in symbol structure (empty children arrays, updated selectionRange values) - All quality tests now passing
Summary
This PR implements batch workspace loading with compression and on-demand cross-file reference resolution to significantly improve workspace loading performance and reduce queue pressure.
Key Changes
🚀 Batch Workspace Loading with Compression
fflatedidOpentasksbatchSizesetting (default: 100 files per batch)⚡ On-Demand Cross-File Reference Resolution
addSymbolTable, cross-file references deferredresolveCrossFileReferencesForFile()method for resolving cross-file references when neededDiagnosticProcessingServiceto resolve cross-file refs before generating diagnostics🔧 Queue Management Optimizations
Priority.Normalto avoid saturating high-priority queue🧪 Testing & Quality
📊 Performance Settings UI
batchSize,maxConcurrency,yieldInterval,yieldDelayMs, andenabledTechnical Details
Architecture Changes
didOpenrequests to structured batch requests with compressionFiles Changed
workspace-loader.ts: Batch loading implementationWorkspaceBatchHandler.ts: Server-side batch processingApexSymbolManager.ts: On-demand cross-file resolutionDiagnosticProcessingService.ts: On-demand resolution integrationBenefits
Testing
@W-20742893@