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

add read/writePointer to be used in threads (de)serializePointer #804

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BTNC
Copy link
Contributor

@BTNC BTNC commented Oct 19, 2016

Hi,

Added read/writePointer together with a test case 'readWritePointer' in test/test_sharedmem.lua. The implementation of (de)serializePointer in threads will be replaced with these new functions instead of relying on readWriteLong/Double.

Travis-ci fail for osx with following error during test, but the compilation does succeed.
/Users/travis/torch/install/bin/luajit: /Users/travis/torch/install/share/lua/5.1/torch/init.lua:13: cannot load '/Users/travis/torch/install/lib/lua/5.1/libtorch.so'

Thanks,

@soumith
Copy link
Member

soumith commented Oct 19, 2016

thanks. dont worry about the OSX build, it's been failing on travis, i have to take a look at it.

Copy link
Contributor

@gchanan gchanan left a comment

Choose a reason for hiding this comment

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

do we need to declare this for all File types, or can it just be in MemoryFile? Seems only useful there.

I haven't thought through if this complicates the implementation; have you considered it?

@@ -68,6 +68,9 @@ If a `Storage` is given, the method will attempt to read a number of elements
equals to the size of the given storage, and fill up the storage with these elements.
The number of elements actually read is returned.

A convenient method exists to read one pointer as a integer: `[integer] readPointer()`. It reads
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rename convenient to convenience.

@@ -109,6 +112,9 @@ in the storage.

These methods return the number of elements actually written.

A convenient method exists to write one pointer: `writePointer(integer)`. It writes
Copy link
Contributor

Choose a reason for hiding this comment

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

here too.

@BTNC
Copy link
Contributor Author

BTNC commented Mar 25, 2017

Yes, it seems only useful for MemoryFile. However if remove this from DiskFile, it would be against the purpose of virtual table and will complicate the implementation and maintenance. By using virtual table (as in c++), it is not expected some sub class have null function pointers in virtual table. To remove this from DiskFile, the function pointers have to be initialized as null pointers and the callers have to check if the function pointers are null pointers before calling them. This special logic will make the implementation less straightforward and less maintainable. Besides, it will add unnecessary null pointer check for the normal case, the MemoryFile case.

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.

3 participants