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

I/O with page cache simulation model #199

Closed
wants to merge 22 commits into from

Conversation

dohoangdzung
Copy link

  • Block, MemoryManager classes are added.
  • List of MemoryManager objects is added to Simulation.
  • readFromHost, writeToHost is added to Simulation.

void readFromHost(WorkflowFile *file, double n_bytes, std::string mountpoint, std::string hostname);
void writeToHost(WorkflowFile *file, std::string mountpoint, std::string hostname);
void writeToHost(WorkflowFile *file, double amount, std::string mountpoint, std::string hostname);
MemoryManager* getMemoryManagerByHost(std::string hostname);
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I think I understand these read/write methods are not doing anything over the network, correct? These are supposed to be local reads from the local memory manager, which maintains a cache for local files? If this is the case, I think that the name is a bit confusing. How about renaming: readFromMemoryCache() or something like that, without passing a hostname argument? Then also rename getMemoryManagerByHost() as simply getMemoryManager() (without a hostname argument). A service can always call Service::getHostname() to find out the name of the host its on. But perhaps I am wrong. It's just that "readFromHost(..., std::string hostname)" really looks like it's for a remote read...

Copy link
Author

@dohoangdzung dohoangdzung Sep 16, 2020

Choose a reason for hiding this comment

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

  • Yeah I agree that readFromHost/writeToHost is somehow confusing, but readFromMemoryCache sounds like it simply reads from memory, so how about readWithMemoryCache/writeWithMemoryCache.
  • The reason I need to pass the hostname to these functions is to know which host we're reading/writing data (they're functions in Simulation). And then I need to call getMemoryManagerByHost(hostname) to get the MemoryManager on that host (from the list of memory managers)
  • Another minor thing is that I should change filename in Block to fileId, so it will be consistent with WRENCH.

Thanks for your quick review by the way! I hope that this can help answer to your concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, i think that readWithMemoryCache/writehWithMemoryCache is probably good for now.

About the hostname, I am confused. I mean, these methods would be called by a running process/actor/service, so your reading from that same host. The only time you need to pass a host name is when you need to do something on a host that's not the one you're currently running on. I guess, what I am asking, is, a process running on "host1" would ever need to call getMemoryManager("host2") or readWithMemoryCache("host2")? If yes, then great. But if not, then there is no need for the host argument. Let me know what you think.

And sure, fileid is probably better.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah you're right that the methods should ideally be called from the same service running on the same host, so hostname is not necessary.
The reason I did that is, the current code in FileTransferThread calls to read/write methods in Simulation, so I did the same thing, write read/write methods in Simulation. I thought that it would have less impact on current code.
So technically, you can call read/write from another the host but the network time is not simulated in our model, so it doesn't work.
I think a better place for read/write methods is in a service that can read/write data (I'm not sure if it is StorageService or simply Service)

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but then let's remove the host name argument, since every call will should look like: f(...., Service::getHostname()). It will be too tempting to call these methods for a remote host, which as you say is not simulated. For instance, the "compute()" method doesn't take a hostname either, even though we could technically do a compute("other host") call (which is, as in your case, not simulated).

As far as where to put these methods, it's a bit tricky. We tend to put methods that just simulated some use of the hardware in the Simulation method. But these are a bit different. For now, Simulation is ok. Another option is to make these methods methods in the MemoryManager class?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I am getting confused here, but in the code of these methods, what's the problem with looking up the hostname? I mean, what I am saying is, instead of:

your_method(...., std::string hostname) { ....}

we can have:

your_method(...) { std::string hostname = S4U_getHostname(); ... }

Is this wrong?

Copy link
Author

@dohoangdzung dohoangdzung Sep 17, 2020

Choose a reason for hiding this comment

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

Yeah I think my problem is I don't know the context of the "calling process" and I'm not sure I can get the correct hostname with Simulation::getHostName(). So I did it that way to make sure it works.

For example, if the call is from FireTransferThread::sendLocalFileToNetwork -> Simulation::readWithCache(), what will be the returned value Simulation::getHostName() called inside Simulation::readWithCache()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that makes sense (by the way, we're arguing about a small detail here, I mean overall everything looks awesome).

In taht example you give, the host name you'll get will be the name of the host on which the calling file transfer thread runs, that is, the source of the transfer. By the way, if you run the simulation with --wrench-full-log, you'll see for each process what hosts it runs on (which is what gethostName() returns).

How about this? You run your code, but add a check in your methods (is hostname = getHostName()), and see if you see any problems. I am 99.99% sure that getHostName() returns exactly what you passed as the hostname argument in all situations. I am saying 99.99% because WRENCH has become complicated ;)

Copy link
Author

Choose a reason for hiding this comment

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

Sounds great! I tried and it works, actually there is only 1 host in my experiment, but you know the framework.
So I think I'll fix as we discussed and make another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good. and if, unexpectedly, it doesn't work, then we'll revert and you'll have my apologies :) Can't wait to see this merged in.

Copy link
Contributor

@henricasanova henricasanova left a comment

Choose a reason for hiding this comment

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

I've entered a comment about the naming of the methods. See what you think. But overall it looks good to me. One thing we should do at some point, once this is merged in, is write wrench::S4U_xxx wrappers, so that there is no "raw" s4u calls at all. But there are other places in WRENCH where we do have direct s4u calls without a wrapper anyway, so this is totally fine for now.

Let me know what you think about my renaming methods comment

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

Successfully merging this pull request may close these issues.

3 participants