Skip to content
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

Improve esbuild’s DX during performance profiling #1236

Closed
wants to merge 1 commit into from

Conversation

iamakulov
Copy link

@iamakulov iamakulov commented May 2, 2021

When you’re profiling esbuild’s bundle initialization, you might stumble upon a whole bunch of (anonymous) functions:

Screen Shot 2021-05-02 at 20 43 53

Frequently, many of these functions are esbuild’s own functions which don’t have a name:

Screen Shot 2021-05-02 at 20 43 59

This PR gives these functions a name and improves the debugging experience. Here’s how the first flamegraph looks after the change:

Screen Shot 2021-05-02 at 20 46 32

Minification / bundle size were not considered. Also, if you’d prefer an alternative function name, I’d be happy to update the PR!

@evanw
Copy link
Owner

evanw commented May 2, 2021

Minification / bundle size were not considered

This is indeed the reason why these are arrow functions. Are you profiling minified or unminified code?

Edit: The reason why I'm asking is that for profiling, ideally each CommonJS wrapper would also be named with something that communicates which module it represents. That's a non-starter for minified code though.

@iamakulov
Copy link
Author

Are you profiling minified or unminified code?

Unminified! I don’t have any DX expectations when debugging minified code.

@iamakulov
Copy link
Author

iamakulov commented May 3, 2021

Ideally each CommonJS wrapper would also be named with something that communicates which module it represents

That’s true! The second half of all (anonymous) functions in the flamegraph above is this:

__commonJS((exports, module) => { ... }))
//         ^

If that function’s name matched the file name, that’d be great. (Perhaps an algorithm similar to what esbuild uses for entrypoint names may be used? To avoid having lots of functions called index.)

// ../../../node_modules/semver/index.js
var require_1 = __commonJS(function semver(exports, module) {})

// ../../../node_modules/semver/getHash.js
var require_2 = __commonJS(function getHash(exports, module) {})
// or?
var require_2 = __commonJS(function semver_getHash(exports, module) {})
// or?
var require_2 = __commonJS(function semver_getHash(exports, module) {})

For comparison, webpack uses full module paths (relative to the project directory) for chunk names during development. This corresponds to optimization.moduleIds: 'named':

/******/ (() => { // webpackBootstrap
/******/ 	var __webpack_modules__ = ({

/***/ "./a/index.js":
/*!********************!*\
  !*** ./a/index.js ***!
  \********************/
/***/ ((__unused_webpack_module, __webpack_exports__, __webpack_require__) => {

"use strict";
__webpack_require__.r(__webpack_exports__);
/* harmony export */ __webpack_require__.d(__webpack_exports__, {
/* harmony export */   "a": () => (/* binding */ a)
/* harmony export */ });
const a = 5;


/***/ }),

/***/ "./node_modules/moment/locale/af.js":
/*!******************************************!*\
  !*** ./node_modules/moment/locale/af.js ***!
  \******************************************/
/***/ (function(__unused_webpack_module, __unused_webpack_exports, __webpack_require__) {

//! moment.js locale configuration
//! locale : Afrikaans [af]
//! author : Werner Mollentze : https://github.com/wernerm

;(function (global, factory) {
    true ? factory(__webpack_require__(/*! ../moment */ "./node_modules/moment/moment.js")) :
   0
}(this, (function (moment) { 'use strict';
...

@evanw
Copy link
Owner

evanw commented May 3, 2021

I'm planning to address this by also naming the per-module function wrappers, and by making sure that this feature doesn't impact non-minified size. My PR for that is here: #1244.

@evanw evanw closed this in #1244 May 3, 2021
@iamakulov iamakulov deleted the patch-1 branch May 4, 2021 14:35
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.

None yet

2 participants