Skip to content

Conversation

@Pho3niX90
Copy link
Contributor

addition of ultimate express https://github.com/dimdenGD/ultimate-express

…se helper

- removed writeResponse function and headerTypes constant for direct header and response handling
- refactored routes to set headers and send responses inline, making code more declarative
- improved readability and reduced abstraction in response handling for each route
"tests": [{
"default": {
"json_url": "/json",
"plaintext_url": "/plaintext",
Copy link
Contributor

@joanhey joanhey Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only json and plaintext test here !!
It's the same with PostgreSQL. !!!

Copy link
Contributor Author

@Pho3niX90 Pho3niX90 Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not fully understanding this comment. Are you saying that there is something missing, or, that the only 2 properties in default should be json_url and plaintext_url?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each bench it's necessary to first create the docker image.
And later run the tests. Time is $$ and energy.

Do you think that running alone json and plaintext, and later again with Postgres will exist a difference.
Of course you can try it, because if it so the framework have a problem.

But please don't repeat tests.
If you need more info, ask me.
Thank you.

"orm": "Full",
"os": "Linux",
"database_os": "Linux",
"display_name": "ultimate-express",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3th Ultimate-express.
Please add in the name to display [normal, charka, mongo, ... ]

res.contentType('text/plain');
res.status(200);

res.send(responseTxt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Content-Length is not specifically needed here and Chunked Transfer encoding is fine, then

 app.get('/plaintext', (req, res) => {
        res.setHeader("Server", "UltimateExpress-GraphQL-MySQL");
        res.setHeader("Content-Type", "text/plain");

        res.send("Hello, World!");
});

would compile it into declarative

@dimdenGD
Copy link
Contributor

Also add app.set("etag", false); in all tests so it doesnt spend most of the time hashing requests

@Pho3niX90
Copy link
Contributor Author

Also add app.set("etag", false); in all tests so it doesnt spend most of the time hashing requests

should be good now, replaced body parser with express. as well

@dimdenGD
Copy link
Contributor

Can I get write access to your fork so I can make the changes

const db = DATABASE ? await import(`./database/${DATABASE}.js`) : null;

const jsonSerializer = sjs({ message: attr("string")});
const jsonSerializer = sjs({ message: attr("string") });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fast-json-steingify is faster than slow-json-stringify with "unsafe" string type https://www.npmjs.com/package/fast-json-stringify

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find a benchmark for that

Copy link
Contributor

@nigrosimone nigrosimone Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made it here https://stackblitz.com/edit/stackblitz-starters-rlrggo

slow-json-stringify: 13.105ms
fast-json-stringify: string: 0.86ms
fast-json-stringify: unfafe: 0.815ms
JSON.stringify: 0.86ms

for small json JSON.stringify is faster, slow-json-stringify is always slow. Side note "fast-json-stringify: unfafe" is raw fast-json-stringify with no special chars support on string (\n, \r, \t, ", \), Hello, World! no need escape.

Use JSON.stringify, this libraries (fast-json-stringify and slow-json-stringify) works better on complex/huge json

slow-json-stringify last update is 4 years ago (maybe is abbandoned)
fast-json-stringify is active and mantained fy Fastify team

The better option is send as raw/stringified string

app.get('/json', (req, res) => {
  res.setHeader('Server', 'UltimateExpress');
  res.setHeader('Content-Type', 'application/json');
  res.end('{"message":"Hello, World!"}');
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must serialize JSON in the request as per test rules. Your benchmark has a bug and doesnt serialize object properly, I fixed and tested it and it seems that fast-json-stringify is still fastest, so I switched it over.

}
})

let isCachePopulated = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be optimized by populating on test init and not on first call, eg:

  let isCachePopulated = false;
  const populate = async() => {
      if (!isCachePopulated) {
        const worlds = await db.getAllWorlds();
        for (let i = 0; i < worlds.length; i++) {
          cache.set(worlds[i].id, worlds[i]);
        }
        isCachePopulated = true;
      }
      return isCachePopulated;
  });
  populate().then(console.log).catch(console.error);

  app.get('/cached-worlds', async (req, res) => {
    res.setHeader('Server', 'UltimateExpress');

    try {
      await populate();
      const count = parseQueries(req.query.count);
      const worlds = new Array(count);

      for (let i = 0; i < count; i++) {
        worlds[i] = cache.get(generateRandomNumber());
      }

      res.json(worlds);
    } catch (error) {
      throw error;
    }
  });
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark has a warm-up period, so it doesn't matter.

@NateBrady23 NateBrady23 merged commit f749c9f into TechEmpower:master Dec 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants