2
\$\begingroup\$

I'm trying to develop code to filter my e-mail with JavaMail, but I doubt it's the best way of doing this. And I want to collect all messages without attachment.

package service.forkinjoin;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.RecursiveAction;

import javax.mail.Message;
import javax.mail.MessagingException;
import javax.mail.Multipart;
import javax.mail.Part;
import javax.mail.internet.MimeBodyPart;

import service.FileUtil;

public class ForkSortMessages extends RecursiveAction {

    private static final long serialVersionUID = -1092415796824205832L;
    private List<Message> listMessages;
    private List<Message> listMessagesToDelete;

    public ForkSortMessages(List<Message> listMessages, List<Message> listMessagesToDelete) {
        this.listMessages = listMessages;
        this.listMessagesToDelete = listMessagesToDelete;
    }

    @Override
    protected void compute() {

        List<RecursiveAction> actions = new ArrayList<>();

        if (this.listMessages.size() <= Runtime.getRuntime().availableProcessors()) {
            try {
                this.separateMessages();
            } catch (MessagingException | IOException e) {
                e.printStackTrace();
            }
        } else {

            int end = this.listMessages.size() / 2;
            actions.add(new ForkSortMessages(this.listMessages.subList(0, end), this.listMessagesToDelete));
            end += this.listMessages.size() % 2 == 0 ? 0 : 1;
            actions.add(new ForkSortMessages(this.listMessages.subList(end, this.listMessages.size()), this.listMessagesToDelete));
            invokeAll(actions);
        }
    }

    private void separateMessages() throws MessagingException, IOException {

        for (Message message : this.listMessages) {

            if ((this.hasNoAttachment(message.getContentType()) || (this.hasNoXmlAttachment(message)))) {
                listMessagesToDelete.add(message);
            }
        }
    }

    private boolean hasNoAttachment(String content) {
        return !content.contains("multipart/MIXED");
    }

    private boolean hasNoXmlAttachment(Message message) throws IOException, MessagingException {

        Multipart multipart = (Multipart) message.getContent();

        for (int i = 0; i < multipart.getCount(); i++) {

            MimeBodyPart mimeBodyPart = (MimeBodyPart) multipart.getBodyPart(i);

            if (Part.ATTACHMENT.equalsIgnoreCase(mimeBodyPart.getDisposition())) {

                if (FileUtil.isXmlFile(mimeBodyPart.getFileName())) {
                    return false;
                }
            }
        }

        return true;
    }
}

So what can I do to improve my code? I've tried to do like Oracle tutorial but I can't.

\$\endgroup\$
8
  • \$\begingroup\$ There is no fork/join, or even Executor code in here... nothing multi-threaded at all.... except a call to invokeAll(...), but there is no invokeAll method here. Where is your code? \$\endgroup\$
    – rolfl
    Feb 7 '14 at 12:15
  • \$\begingroup\$ @rolfl my code is following a example found in Google, if my code isn´t any kind of multi-threaded. Show me a example \$\endgroup\$ Feb 7 '14 at 12:19
  • \$\begingroup\$ @rolfl problably I told wrong things, but I created my code using tutorial found in Google, now I recreated my code, following tutorial of Oracle. \$\endgroup\$ Feb 8 '14 at 10:15
  • \$\begingroup\$ @rolfl: It's in the superclass. \$\endgroup\$
    – palacsint
    Feb 8 '14 at 11:23
  • 1
    \$\begingroup\$ Do you need this code to be multi-process? It seems that the actual work is simply to check if it has no attachments - hardly a reason to fork the work... \$\endgroup\$
    – Uri Agassi
    Feb 8 '14 at 19:17
3
\$\begingroup\$

In order to correctly work in parallel, you must follow the Actor Model, which basically says that each iteration should not share memory when doing their work. In your code, you pass a List<Message> to the child processes, which they are expected to fill. Each child process changes his own copy of the list, which makes the final result unexpected.

To build your actors correctly, each should create its own solution vector, and later its parent process should merge the result of the children, and pass them on (you can see samples here).

If the order of the messages is important to you, even this is not enough, since you don't know which child process will finish first, so the messages in the resulting list will not be in the same order as in the input list.

After all being said and done - you might find out that you do not get better performance, and sometimes even worse performance.

My suggestion is to keep it simple - the actual work you are doing on the array is simple enough - do a straight forward iteration. By they way you describe your use-case - at most you will have to wait a couple of minutes (tens of minutes?) for the initial bulk to be computed, but at least you'll be confident that you have the correct result. I assure you it would take less time to run it synchronously than to develop and debug a fork/join solution...

\$\endgroup\$
3
  • \$\begingroup\$ Ok, you won, I´m going to keep it simple. Later I´m going to read your document, I just wanted to process things much faster. But I need to learn more about Java Parallel \$\endgroup\$ Feb 8 '14 at 21:06
  • \$\begingroup\$ I have one doubt, operation with I/O, are not good to use threads or parallel operations? \$\endgroup\$ Feb 9 '14 at 11:58
  • \$\begingroup\$ There is no I/O in your filtering - you simply look at the content of the message you received... \$\endgroup\$
    – Uri Agassi
    Feb 9 '14 at 12:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.