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

RFC: Remove Ryan's Freelist feature #569

Closed
19h opened this issue Jan 23, 2015 · 24 comments
Closed

RFC: Remove Ryan's Freelist feature #569

19h opened this issue Jan 23, 2015 · 24 comments
Labels
discuss Issues opened for discussions and feedbacks. semver-major PRs that contain breaking changes and should be released in the next major version.

Comments

@19h
Copy link
Contributor

19h commented Jan 23, 2015

Ho,

Ryan introduced FreeList in 2010 (landed in aadce8e) and is has been usable ever since.

However, it is not documented nor is it used in core-libraries and IMHO the code doesn't qualify for a core-module.

var fl = new (require("freelist").FreeList) ("foo", 10, function nop(){});
fl.free(123);
fl.alloc();

Comments?

@bnoordhuis
Copy link
Member

lib/_http_common.js uses it for caching parser objects. I'd be okay with moving lib/freelist.js into lib/_http_common.js if we knew for sure that no one was using it but GH search suggests that there are at least a few projects that do.

@19h
Copy link
Contributor Author

19h commented Jan 23, 2015

I'd go with moving it. I looked at each case for the first 10 pages of results and most uses of the module are either 1) node wrappers that wrap1 _mock_ freelist for .. I guess the sake of it, 2) code by substack.. and this is the best part: substack/node-browserify/1223984 he's actually written an override for freelist, so it's not our freelist.

1 They're NOP'ing the method, so it's actually just a mock.

@vkurchatkin
Copy link
Contributor

we need truly private internal modules for these kind of things.

@19h
Copy link
Contributor Author

19h commented Jan 23, 2015

On the first 20 pages, I found one actual use of the module: zxcabs/node-proxy-cache/bf3a1a4.

We could move FreeList to http#FreeList and document it, then deprecate _freelist_ as a module and warn the user to move on to http#FreeList.

@tellnes
Copy link
Contributor

tellnes commented Jan 25, 2015

If it were up to me I would put it on npm, deprecated it and removed it in a major version update at a later date.

@19h
Copy link
Contributor Author

19h commented Jan 25, 2015

Did this, done that: npm/freelist. Deprecation isn't that easy though, because #http requires it and we cannot simply move it there because it's a pain in the ass to move cross-require http from iojs#freelist just to have it. I'm not sure about this.. the last resort would be to simply document that feature for the time being..

@Fishrock123 Fishrock123 added the discuss Issues opened for discussions and feedbacks. label Jan 30, 2015
@chrisdickinson
Copy link
Contributor

freelist gets the same pass sys does – it costs nothing (or next to nothing) to maintain, but could break downstream users if we remove it outright. I'd be alright with:

  • Copy the existing freelist.js to _freelist.js.
  • Make _http_common.js require _freelist.js.
  • Issue a deprecation notice if freelist.js is required.
  • The next time a major version is on the table, we may consider revisit ripping out sys and freelist.

@chrisdickinson chrisdickinson added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 4, 2015
@seishun
Copy link
Contributor

seishun commented Feb 14, 2015

Was freelist ever documented? Backwards compatibility is great but we should draw the line somewhere. We shouldn't have to perpetually support an internal feature just because someone decided to use it.

@petkaantonov
Copy link
Contributor

lib/_http_common.js uses it for caching parser objects. I'd be okay with moving lib/freelist.js into lib/_http_common.js if we knew for sure that no one was using it but GH search suggests that there are at least a few projects that do.

Pretty sure it could be doing more harm than good so we should re-consider even using it, v8 gc has changed a lot in the last 5 years

@Fishrock123
Copy link
Contributor

@chrisdickinson can you run this against your static npm analyzer thing?

@Trott
Copy link
Member

Trott commented Jul 14, 2015

There is now a micro-optimization PR on FreeList.

If (as @petkaantonov suggests), FreeList may be hurting performance and not helping, a benchmark showing that would be useful...

@mikeal
Copy link
Contributor

mikeal commented Jul 14, 2015

Being that this is exposed publicly we should work it through the deprecation process (which I believe @chrisdickinson is in the process of documenting and testing). This may be a good candidate to do alongside the deprecation of sys since it probably has similar outstanding usage.

@thefourtheye
Copy link
Contributor

@mikeal sys and smalloc already issue deprecation warning.

@mikeal
Copy link
Contributor

mikeal commented Jul 14, 2015

@thefourtheye the process of actually removing publicly exposed API is more than just printing a warning for a release.

thefourtheye added a commit to thefourtheye/io.js that referenced this issue Jul 14, 2015
As per the dicussion in nodejs#569,
this patch issues a deprecation warning when freelist modeule is
required.
@trevnorris
Copy link
Contributor

@mikeal The act of adding deprecation warnings, in node, is no indication that the feature will ever be removed.

@mikeal
Copy link
Contributor

mikeal commented Jul 14, 2015

@trevnorris i know, but we've been talking about a deprecation process that would actually remove public API at some point and printing a warning is part of that.

@trevnorris
Copy link
Contributor

At thanks. Missed that context skimming over the issue.

thefourtheye added a commit that referenced this issue Jul 18, 2015
As per the dicussion in #569,
this patch issues a deprecation warning when freelist module is
required. A test file for freelist is also added.

PR-URL: #2176
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Brendan Ashworth <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Nov 9, 2015

It looks like this was deprecated back in io.js. Is there more to be done here? Are we actually going to remove freelist, or can this issue by closed?

@mikeal
Copy link
Contributor

mikeal commented Nov 9, 2015

Looks like we have a deprecation warning now, would be cool if we could close this and have it automatically re-open in a year to consider removing ;)

@cjihrig
Copy link
Contributor

cjihrig commented Nov 9, 2015

I'm going to close. @geek would you mind being our watchdog ;-)

This was referenced Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

No branches or pull requests