Skip to content

perpetual: new module for mapping value types into memory #3625

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

Closed
wants to merge 36 commits into from

Conversation

sdegtiarev
Copy link
Contributor

I rewrote std.perpetual module to be based on top of std.mmfile.
It becomes clear that high-level data handling should be separate from low-level shared memory allocation. In addition to architectural advantages, this also clearly answers the question: what new the perpetual module added to existing functionality.

@quickfur
Copy link
Member

quickfur commented Sep 2, 2015

Why std.perpetual instead of std.persistence? Persistence is well-established terminology for what this module does, "perpetual" sounds strange.

@@ -0,0 +1,192 @@
module std.perpetual;
Copy link
Member

Choose a reason for hiding this comment

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

Modules need to have proper ddoc header.

Copy link
Member

Choose a reason for hiding this comment

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

module std.experimental.perpetual;

And please remove the extra perpetual folder - it's a module not a package.

@quickfur
Copy link
Member

quickfur commented Sep 2, 2015

Also, please conform to the Phobos style guide: http://dlang.org/dstyle.html (note especially the last section on Additional Requirements for Phobos).

@quickfur
Copy link
Member

Since this PR is proposing to add a new Phobos module, I think it should at least be discussed on the forum and go through the Phobos review process.

@sdegtiarev
Copy link
Contributor Author

@quickfur Certainly, I still need to fix some urgent issues, plus, a little more detailed documentation would be useful

@DmitryOlshansky
Copy link
Member

Another thing to note - get rid of merge commits and use git rebase to squash minor edits and things like moving files around.

@JackStouffer
Copy link
Member

Why are there two PRs for the same thing? #3562




private import std.exception;
Copy link
Member

Choose a reason for hiding this comment

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

Please put all imports at the top of the file.

@JackStouffer
Copy link
Member

Also, please change all of your tab indentation to four spaces.

@sdegtiarev
Copy link
Contributor Author

Will do, thanks.

I love this, will use: static assert(!is(typeof(..expr..)));

On Sat, 9/19/15, Jack Stouffer notifications@github.com wrote:
...

core
Makefile
perpetual.html

Copy link
Member

Choose a reason for hiding this comment

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

Is this file needed once it would be in Phobos? All the other modules are fine without this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, of course not.

@JackStouffer
Copy link
Member

I would suggest this be closed until this module is ready for prime time. Right now all it is doing is using autotester time needlessly.

@sdegtiarev
Copy link
Contributor Author

It is actually ready, I don't see what else I can do with it.
Changed status to "voting"

// scalar value or static array
else
{
alias Element=T;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: whitespace

@wilzbach
Copy link
Member

It is actually ready, I don't see what else I can do with it.
Changed status to "voting"

@sdegtiarev can you please make the NG annoucement then?
@JackStouffer do you volunteer as a review manager for the voting?
(don't forget to update the review queue based on the voting)

@JackStouffer
Copy link
Member

Sorry, I don't have time right now.

@wilzbach
Copy link
Member

Changed status to "voting"
@sdegtiarev can you please make the NG annoucement then?

I wouldn't mind handling the review - so move this forward and start the discussion ;-)

@dnadlinger
Copy link
Member

I think this is a prime candidate for Dub. There is a gazillion ways of interfacing memory-mapped I/O, and this is just one of them.

@sdegtiarev
Copy link
Contributor Author

I started discussion thread.
https://forum.dlang.org/post/stesmdubyyldrseepqqz@forum.dlang.org

Hopefully, it will push things forward.

On Tue, 5/10/16, Sebastian Wilzbach notifications@github.com wrote:
I wouldn't mind handling the review - so move this
forward and start the discussion ;-)

@wilzbach
Copy link
Member

@sdegtiarev

  1. do you mind running "dfmt" on your module? You miss a lot of whitespaces etc.
  2. can you squash your commits into one?

@klickverbot i will stay neutral as review manager - my point is that such pending PRs don't help anyone. Either dub or Phobos, but not pending ;-)

@mleise
Copy link
Contributor

mleise commented May 11, 2016

What happens with pointers in the embedded objects when the file is restored from file or used by multiple processes?

@sdegtiarev
Copy link
Contributor Author

No, pointers are not of course allowed in stored data, this is general limitation for such code. You have to use serialization if you need such functionality.

On Wed, 5/11/16, Marco Leise notifications@github.com wrote:

What happens with pointers in the
embedded objects when the file is restored from file or used
by multiple processes?

@sdegtiarev
Copy link
Contributor Author

Believe it or not, I can't build dfmt... One of my machines has 2Gb ram (+swap) and dmd reports insufficient memory, another one has 8Gb ram but version dmd-2.67 and reports compilation error: template 'AliasSeq' is not defined. Incredible.

On Wed, 5/11/16, Sebastian Wilzbach notifications@github.com wrote:

Subject: Re: [dlang/phobos] perpetual: new module for mapping value types into memory (#3625)
To: "dlang/phobos" phobos@noreply.github.com
Cc: "Sergei Degtiarev" sdegtiarev@yahoo.com, "Mention" mention@noreply.github.com
Date: Wednesday, May 11, 2016, 6:40 AM

@sdegtiarev

  1. do you mind running "dfmt" on your module? You
    miss a lot of whitespaces etc.

  2. can you squash your commits into one?

@klickverbot i will
stay neutral as review manager - my point is that such
pending PRs don't help anyone. Either dub or Phobos, but
not pending ;-)


You are receiving this because you were mentioned.
Reply to this email directly or view
it on GitHub

@wilzbach
Copy link
Member

Believe it or not, I can't build dfmt... One of my machines has 2Gb ram (+swap) and dmd reports insufficient memory, another one has 8Gb ram but version dmd-2.67 and reports compilation error: template 'AliasSeq' is not defined. Incredible.

Have you tried with a recent dmd version like 2.070 or 2.071?

@dnadlinger
Copy link
Member

dnadlinger commented May 15, 2016

@wilzbach: You are right, pending PRs don't help anybody. I'm thus closing it for now.

As stated above, it strikes me as fundamentally too narrow in scope, and its quality of implementation isn't particularly high. The API would be of the smallest in Phobos, yet the documentation is inexcusably lacking (if the module had been properly added to the build system, this would be obvious; see e.g. the single 100+ lines documented unit test).

The conclusion of the various standard library discussions at DConf was that we want things in the standard library that are either pivotal to the implementation of a consistent set of other functionality (containers, Big-O, etc.), or exploit D's potential for the task in question to its fullest (e.g. std.regex). This module matches neither of the criteria.

Of course, you are still free to decide and push this through the review process. In that case, however, please make sure to follow the format of the previous iterations. There isn't a lot of formal criteria, but the review manager should be the one to announce the begin of the review on both newsgroups, together with a summary of the module, links to source code and documentation, the end of the review period, particular points of interest/discussion, etc. Hopefully there is more on this somewhere the wiki, but the basic idea is to act as a mediator and manager to make it easy for community members to start evaluating the library.

@sdegtiarev: Please don't become discouraged by this. I've had a look at your other code here on GitHub, and you unquestionably have some interesting ideas. For a language standard library, however, the barrier of entry is quite a bit higher than "could be useful for somebody". We've previously accepted code that we shouldn't have, and are still busy sorting out the consequences.

At DConf, we also discussed that we want to introduce a process by which we can highlight solid libraries to stand out from the anonymous crowd, as it were. The details are still being worked out, but maybe this module could be one of the first examples.

@dnadlinger dnadlinger closed this May 15, 2016
@sdegtiarev
Copy link
Contributor Author

dmd 2.07 is as memory greedy as as older version. Will try to install and build on more powerful machine tomorrow.

On Sun, 5/15/16, Sebastian Wilzbach notifications@github.com wrote:

Have you tried with a recent dmd version like 2.070 or
2.071?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants