Improving Matter generation time for Simplicity Studio #1657
Conversation
… and templates meta data to use what is in the slc agrs json file during the file open operation JIRA: ZAPP-1656
Summary of ChangesHello @brdandu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a mechanism to improve Matter generation time within Simplicity Studio by optimizing how ZCL and template metadata are loaded. When opening a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an optimization for loading ZCL and template metadata by using a slc_args.json file, which should improve generation time. The overall approach is sound. My review includes several suggestions to enhance performance, maintainability, and consistency with the existing codebase. These include using asynchronous file operations, refactoring duplicated code, using the project's standard logger, and optimizing database queries after package loading.
| await zclLoader.loadZcl(db, packagePath) | ||
| } else if (packageType === dbEnum.packageType.genTemplatesJson) { | ||
| // Load template package using generation engine | ||
| const genEngine = require('../generator/generation-engine.js') |
There was a problem hiding this comment.
For better performance and code organization, require statements should be at the top of the file. This allows modules to be loaded once when the module is first required, and it makes dependencies clearer.
| if (packageType === dbEnum.packageType.zclProperties) { | ||
| // Load ZCL properties package | ||
| await zclLoader.loadZcl(db, packagePath) | ||
| } else if (packageType === dbEnum.packageType.genTemplatesJson) { | ||
| // Load template package using generation engine | ||
| const genEngine = require('../generator/generation-engine.js') | ||
| await genEngine.loadTemplates(db, [packagePath]) | ||
| } | ||
|
|
||
| // Re-query after loading | ||
| existingPackage = await queryPackage.getPackageByPathAndType( | ||
| db, | ||
| packagePath, | ||
| packageType | ||
| ) |
There was a problem hiding this comment.
After loading a package, the code re-queries the database using the package path and type. This is inefficient. The loadZcl and loadTemplates functions return a context object containing the packageId of the newly loaded package. You can use this ID to fetch the package directly with queryPackage.getPackageByPackageId, which is more efficient.
| if (packageType === dbEnum.packageType.zclProperties) { | |
| // Load ZCL properties package | |
| await zclLoader.loadZcl(db, packagePath) | |
| } else if (packageType === dbEnum.packageType.genTemplatesJson) { | |
| // Load template package using generation engine | |
| const genEngine = require('../generator/generation-engine.js') | |
| await genEngine.loadTemplates(db, [packagePath]) | |
| } | |
| // Re-query after loading | |
| existingPackage = await queryPackage.getPackageByPathAndType( | |
| db, | |
| packagePath, | |
| packageType | |
| ) | |
| if (packageType === dbEnum.packageType.zclProperties) { | |
| // Load ZCL properties package | |
| const { packageId } = await zclLoader.loadZcl(db, packagePath) | |
| existingPackage = await queryPackage.getPackageByPackageId(db, packageId) | |
| } else if (packageType === dbEnum.packageType.genTemplatesJson) { | |
| // Load template package using generation engine | |
| const genEngine = require('../generator/generation-engine.js') | |
| const { packageIds } = await genEngine.loadTemplates(db, [packagePath]) | |
| if (packageIds && packageIds.length > 0) { | |
| existingPackage = await queryPackage.getPackageByPackageId( | |
| db, | |
| packageIds[0] | |
| ) | |
| } | |
| } |
|
|
||
| return existingPackage | ||
| } catch (error) { | ||
| console.warn(`Failed to load package ${packagePath}: ${error.message}`) |
There was a problem hiding this comment.
The project uses a pino logger accessed via env.log* functions for structured and consistent logging. Instead of console.warn, please use env.logWarning. This also applies to console.log on line 53. You will need to import the env module at the top of the file.
| console.warn(`Failed to load package ${packagePath}: ${error.message}`) | |
| env.logWarning(`Failed to load package ${packagePath}`, error) |
| try { | ||
| if (fs.existsSync(slcArgsFile)) { | ||
| const slcArgsData = await fsp.readFile(slcArgsFile, 'utf8') | ||
| slcArgs = JSON.parse(slcArgsData) | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to read slc_args.json: ${error.message}`) | ||
| } |
There was a problem hiding this comment.
The use of fs.existsSync is synchronous and can block the event loop, which should be avoided in an async function. A better approach is to attempt reading the file with fsp.readFile and handle the ENOENT (file not found) error specifically if it occurs. This keeps the operation fully asynchronous.
| try { | |
| if (fs.existsSync(slcArgsFile)) { | |
| const slcArgsData = await fsp.readFile(slcArgsFile, 'utf8') | |
| slcArgs = JSON.parse(slcArgsData) | |
| } | |
| } catch (error) { | |
| console.warn(`Failed to read slc_args.json: ${error.message}`) | |
| } | |
| try { | |
| const slcArgsData = await fsp.readFile(slcArgsFile, 'utf8') | |
| slcArgs = JSON.parse(slcArgsData) | |
| } catch (error) { | |
| if (error.code !== 'ENOENT') { | |
| console.warn(`Failed to read or parse slc_args.json: ${error.message}`) | |
| } | |
| } |
| if (category.includes(`'${dbEnum.helperCategory.zigbee}'`)) { | ||
| // Load Zigbee ZCL properties if specified | ||
| let zclPackage = await ensurePackageLoaded( | ||
| db, | ||
| slcArgs[dbEnum.slcArgs.zigbeeZclJsonFile], | ||
| dbEnum.packageType.zclProperties | ||
| ) | ||
| if (zclPackage) { | ||
| zclProperties.push(zclPackage) | ||
| } | ||
|
|
||
| // Load Zigbee templates if specified | ||
| let templatePackage = await ensurePackageLoaded( | ||
| db, | ||
| slcArgs[dbEnum.slcArgs.zigbeeTemplateJsonFile], | ||
| dbEnum.packageType.genTemplatesJson | ||
| ) | ||
| if (templatePackage) { | ||
| zclGenTemplates.push(templatePackage) | ||
| } | ||
| } | ||
|
|
||
| if (category.includes(`'${dbEnum.helperCategory.matter}'`)) { | ||
| // Load Matter ZCL properties if specified | ||
| let zclPackage = await ensurePackageLoaded( | ||
| db, | ||
| slcArgs[dbEnum.slcArgs.matterZclJsonFile], | ||
| dbEnum.packageType.zclProperties | ||
| ) | ||
| if (zclPackage) { | ||
| zclProperties.push(zclPackage) | ||
| } | ||
|
|
||
| // Load Matter templates if specified | ||
| let templatePackage = await ensurePackageLoaded( | ||
| db, | ||
| slcArgs[dbEnum.slcArgs.matterTemplateJsonFile], | ||
| dbEnum.packageType.genTemplatesJson | ||
| ) | ||
| if (templatePackage) { | ||
| zclGenTemplates.push(templatePackage) | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for loading packages for Zigbee (lines 119-139) and Matter (lines 141-161) is nearly identical. This duplication can be avoided by extracting the logic into a helper function.
For example, you could create a function like loadPackagesForCategory(db, slcArgs, category, zclProperties, zclGenTemplates) that takes the category ('zigbee' or 'matter') and populates the zclProperties and zclGenTemplates arrays.
This would make the code more concise and easier to maintain.
…le beside zap JIRA: ZAPP-1656
1 participant
Using slc args json file when opening .zap files and changing the zcl and templates meta data to use what is in the slc agrs json file during the file open operation
JIRA: ZAPP-1656