Skip to content

Conversation

@lvitals
Copy link

@lvitals lvitals commented Apr 9, 2025

Performance Improvements:

  • Cached frequently used functions and methods
  • Optimized string operations
  • Reduced unnecessary table creations

Code Quality:

  • Centralized regex patterns as constants
  • Improved error handling
  • Added safety checks for edge cases
  • Reduced code duplication
  • Standardized code formatting

Maintainability:

  • Better code organization
  • More consistent style
  • Clearer separation of concerns

- Cache frequently used functions for better performance
- Centralize regex patterns in constants
- Add better error handling and edge case protection
- Improve code organization and readability
- Reduce code duplication
- Add more safety checks
- Standardize code formatting
- Optimize string operations
@tomasguisasola
Copy link
Member

Have you tested the code? I think I found some mistakes:

  1. At line 121, the variable _ in your code is a global, but it was a local in the original;
  2. At line 158, you could also use a constant for the pattern (just a suggestion);
  3. Also at line 158, you should not use %- when using string.find with true as the fourth value, since it turns off the pattern matching facilities (the same occurs at line 160);
  4. At lines 159, 161 and 163, the new call input(length) is not the same as the original wsapi_env.input:read(length), since the first argument (wsapi_env.input) is absent: you might correct that!
  5. You also make a different metatable, although with the same content, for each :new call

This commit addresses several issues in the `wsapi.request` module, including bug fixes, performance improvements, and code quality enhancements, based on an initial code review.

- **Performance**: Implemented metatable sharing for the `params` and `cookies` tables in `request.new()`. This avoids creating new metatables for each request, significantly reducing memory consumption and CPU usage under high load.

- **Bug Fixes**:
  - Corrected a bug where `input:read(length)` was being called as a function (`input(length)`), causing errors in POST data parsing.
  - Fixed an issue where `post_data` was not being correctly initialized for empty POST requests with non-form-encoded content types.
  - Ensured that all variables are properly scoped to avoid accidental globals.

- **Code Quality**:
  - Replaced hardcoded content type strings with constants for better readability and maintainability.
  - Removed unnecessary character escaping in string patterns.

A new test file (`tests/test_metatable.lua`) has been added to verify the metatable sharing optimization and includes a performance benchmark.
@lvitals
Copy link
Author

lvitals commented Oct 21, 2025

Hi @tomasguisasola,

Thank you so much for the detailed and insightful feedback! You've pointed out some very important issues.
I've pushed a new version that addresses all of them.

Here’s a summary of the corrections based on your comments:

  • Global Variable (_): You were right about the global _ on line 121. I've fixed this by ensuring the
    variable is properly declared as a local within its scope.

  • Pattern Constants and %- Usage: Great suggestion for line 158. I have refactored the code to use
    constants for the content-type strings, which improves readability. I also removed the unnecessary %-
    escaping, as you correctly pointed out that it's not needed when string.find is used with the plain flag.

  • input(length) Call: You spotted a critical bug here. This was a major issue that is now fixed. I've
    corrected the logic to ensure wsapi_env.input:read(length) is called as a method, passing the self argument
    correctly.

  • Shared Metatables: This was a great catch for performance. I've refactored the request.new function to
    create the metatables for params and cookies only once. They are now shared across all request instances,
    which significantly reduces memory allocation and CPU overhead, especially under high load.

To validate this performance improvement, I've also added a new test file, tests/test_metatable.lua, which
includes a benchmark and an assertion to confirm that the metatables are indeed being shared.

Thanks again for your thorough review! Let me know if you have any other questions or suggestions.

Removed outdated Windows Makefile and installer script.
… handles response bodies. I also expanded the tests to cover more scenarios like POST

  requests and multi-chunk responses. Additionally, I modernized the build system by improving the Makefile and configure script, making the project easier to
    build and install, and removed old installation scripts
…or testing, installation, and uninstallation, as well as to automate the generation of rockspec files
…d user-friendly demonstration of the library's capabilitie
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants